Re: [libmicrohttpd] Log if removing cork is unsuccessful

[Christian Grothoff]

On 01/10/2013 10:40 AM, Václav Pavlín wrote:
> Hi,
>
> I ran Coverity Scan on last version of libmicrohttpd and it found an
> unchecked return value of setsockopt when setting and removing TCP_CORK.
>  From my understanding of TCP_CORK it won't do much harm if setting of
> cork fails, but it might do some if we are not able to remove the cork -
> some data might not be sent.
Not removing the cork is also harmless.
> On the other hand, I don't think the connection should be closed when we
> cannot remove the cork, so I just added message to log, so the user is
> able to identify the issue. (see the attached patch)
The real issue is that your OS must be VERY bugged for setting TCP_CORK 
to succeed and then removing to fail.  As you said, if both fail, we 
just suffer a minor loss of performance.  Now, if ONLY unsetting 
TCP_CORK fails, there is still no data loss.  The worst that will happen 
is that we either don't use TCP corking ever (so always send 
immediately), or that the kernel may at times additionally delay a 
packet by like 100ms.  So the error case is completely harmless either way.

There is also again the question of how to possibly handle this 
condition (other than by logging).  As it is harmless, a log message may 
confuse more than help -- after all, things are working. So I actually 
consciously decided against doing anything in this particular case.  We 
have a similar 'setsockopt' case for IPV6_V6ONLY in daemon.c; again, the 
failure is "mostly harmless" and actual handling of errors is likely to 
do more harm (as in, break stuff) than ignoring the return value.

> Second thing Coverity is complaining about, is that you can find
> always-false condition in file daemon/daemon.c on line 783:
>
> 782     left = connection->response->total_size -
> connection->response_write_position;
> 783     if (left>  SSIZE_MAX)
> 784             left = SSIZE_MAX; /* cap at return value limit */
>
> As SSIZE_MAX is the max value of left's type, it can never be greater.
>  From the expression assigning value to variable left I would expect left
> should not be less than zero. But I am not sure if I got meaning of
> total_size and response_write_position completely right.
>
> So my question is if we should not rather check left against zero
> instead of SSIZE_MAX?
Coverity is making invalid assumptions.  "left" is of type "off_t", 
which can be 32 bit or 64 bit,
signed or unsigned.  SSIZE_MAX is the maximum value of 'ssize_t', which 
can be 32-bit or 64 bit signed; and these are actually only the 
constraints on _common_ platforms; exotic systems might be even more crazy.

So on some platforms this check may be unnecessary, but not on all.


Happy hacking!

Christian