bsd_tcpip stack - socket lost when connection is closed while on accept queue

["Cameron Taylor" <Cameron dot Taylor at vecima dot com>]

The attached patches fix a bug in the net/bsd_tcpip stack, where, when a
tcp connection is closed before the user code accepts it, the associated
socket is not freed.

In our case, this occurred when a TCP connection is made to the unit and
immediately reset:
Foreign host                eCos host
    ->      TCP SYN       ->
    <-      TCP SYN,ACK   <-
    ->      TCP ACK       ->
    ->      TCP RST       ->

Which occurs during a tcp connect scan or for some management software
probing whether the unit is alive. The nmap command:
nmap -n -sT -p80 10.0.0.150

where '80' is an open TCP port and '10.0.0.150' is the IP address of the
unit, will cause this packet sequence; and, if the RST packet is
processed before the 'accept()' call occurs, accept() returns an error
but the socket is never freed. 

On receiving the RST, tcp_input() calls tcp_close() calls
soisdisconnected() which marks the socket SS_ISDISCONNECTED and
in_pcbdetach() calls sofree(), but the socket is still on the accept
queue, so it is not freed. When accept() runs, it takes the socket off
the accept queue and calls bsd_accept() calls soaccept() calls
tcp_usr_accept() which checks SS_ISDISCONNECTED and returns
ECONNABORTED.

Previously, at this point the socket handle (which was copied to the
return structure) was dropped and an ECONNABORTED returned. The return
structure is ignored on error.

The patches insert soclose() at this point. In the original free bsd
code, this is called several layers down in fdrop(), which is
(correctly) commented out in the port as the file allocation is done at
the higher layer in eCos. Since bsd_accept() takes ownership of the
socket when it takes it off the accept queue, it is appropriate to free
it here.

In minimumdiff.txt, this is the only change.

While I was there, I also noticed the NULL checks for "name" and
"anamelen" are not consistent and are made one extra time, the lower
half of the function is hard to follow due to gotos, there is an extra
"error=0", the now invalid socket handle is "leaked" out the new_fp
structure and finally, an error returned by copyout() would also cause
the socket to be lost. To correct that, I've moved and changed the NULL
checks, deleted the #if 0 sections, turned the unnecessary gotos into
if/else structures and moved the new_fp assignment. Handling an error
returned from copyout would involve a call to soclose and moving the
new_fp assignment after it; but copyout() is just memcpy and always
returns true, so I've called memcpy directly and eliminated the
unnecessary check.

All of these changes are in fulldiff.txt

We have tested the change in our products, triggering the event of
interest as well as regular use of accept. None of the existing test
cases seems to cover this situation, largely because it is a timing
issue.

Cameron Taylor
Vecima Networks

Attachment: mindiff.txt
Description: mindiff.txt

Attachment: fulldiff.txt
Description: fulldiff.txt