[squid-dev] [PATCH] Fix various issues detected by Coverity Scan

[Kinkie]

On Fri, Jul 10, 2015 at 3:40 PM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 11/07/2015 1:21 a.m., Kinkie wrote:
> > On Fri, Jul 10, 2015 at 2:39 PM, Amos Jeffries <squid3 at treenet.co.nz>
> wrote:
> >
> >> On 10/07/2015 3:18 a.m., Kinkie wrote:
> >>> Hi,
> >>>   I'm going through the issues identified by Coverity Scan, in
> >>> chronological order.
> >>> This patch covers 11 defects, mostly unchecked return values. It also
> >>> converts unlinkd to c++ (original defect: using tainted strings). They
> >> have
> >>> all build-tested; unlinkd has been (hand)-run tested.
> >>>
> >>
> >> Audited on IRC, summary below:
> >>
> >> in src/DiskIO/AIO/AIODiskIOStrategy.cc does the change actually do
> >> anything useful?
> >>  - the entire function is dead code due to the return-0 that starts it.
> >>  - thats why I marked that issue as intentional to be ignored.
> >>  - Ive been contemplating making it #if 0 wrapped for coveritys
> pleasure.
> >>
> >
> > Gah, right. #0-ed the whole function body
> >
> >
> >> in src/DiskIO/DiskDaemon/DiskdFile.cc I thought you were going to use
> >> the unlock() result inline for the if-statement, the variable seems
> >> unnecessary since its not even used in the debug statement
> >>
> >
> > I've used a variable for readability; I can refactor no problem. As the
> > variable is const, I expect it to be optimized away anyway. Do you wish
> me
> > to?
> >
> >
> >> in src/debug.cc (or anywhere else) please dont use xstrerror() anymore.
> >>  - use xstrerr(errno) instead. since the debugs() initilization
> >> overwrites errno value.
> >>
> >
> > Done
> >
>
> Meh I mucked up there. THat wont work due to same reason for not using
> xstrerror().
>
> Use a const int xerrno = errno; first, then inside the debugs( ...
> xstrerr(xerrno) ...
>

Doing that in debug.cc; about the others I'll wait for the discussion to
reach a conclusion.


> >
> >>  - also at that point in src/debug.cc the debugs() log file is closed.
> >>   = at least on Windows builds. have to report errors to stderr instead
> >> until we fix debugs() to store in a temporary buffer when waiting on
> >> cache.log existence (that could be an XXX comment).
> >>
> >
> > via std::cerr or some other mechanism?
> >
> >
>
> Good question.
>
> Looking at it closer I see you were only altering inside the for-loop.
> Both the loop and the if-statement need the same checks and output on
> failure.
> It is probably best to add a helper static function that calls
> _db_print_stderr(), _db_print_file(), then _db_print_syslog() in that
> specific order with the same arguments to each.
> Just be careful not to pass va_args around as they are one-use objects.
>

I'm not sure it makes sense: in main, if Debug::log_stderr < 0, stderr gets
redirected to /dev/null. Andif it isn't, debugs() already prints to stderr
via _db_print_stderr; I'm leaving it as-is for now,

[...]


> +1 with the above extra changes.
>

Thanks


-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150712/8b3bd5d8/attachment.html>