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

[Kinkie]

Hi,
  (aligning the other devs)
After some discussion on IRC, it seems that the best path forward is to
prevent the call to strftime nested deep in debugs() from clobbering errno;
any other option has a global impact on the code base and would be a bad
hack.
I'll prepare that in a separate patch to trunk.

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) ...
>
> >
> >>  - 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.
>
>
> >> in src/ipc.cc the conventions Ive been using is to name the sockopt
> >> which failed. ie debugs(..."setsockopt(FOO) failed: " << xstrerr(errno))
> >>
> >
> > Done.
> >
> >
> >> in src/unlinkd_daemon.cc #include statements aphabetical please. helps
> >> avoid a reformat followup commit.
> >> - can you double-check the removal of setbuf() please? AFAIK the
> >> iostreams still need un-buffering and theres no iostreams way to do it.
> >>
> >
> > std::endl flushes the output buffer (from
> > http://en.cppreference.com/w/cpp/io/manip/endl :
> > "Inserts a newline character into the output sequence os and flushes it
> as
> > if by calling os.put(os.widen('\n')) followed by os.flush()."
>
> Doh. Right, and Squid flushes stdin at its end on sending.
>
>
> +1 with the above extra changes.
>
> Amos
>



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