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

[Amos Jeffries]

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