* bsd_tcpip stack - socket lost when connection is closed while on accept queue
@ 2010-02-01 20:53 Cameron Taylor
0 siblings, 0 replies; only message in thread
From: Cameron Taylor @ 2010-02-01 20:53 UTC (permalink / raw)
To: ecos-patches
[-- Attachment #1: Type: text/plain, Size: 2907 bytes --]
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 #2: mindiff.txt --]
[-- Type: text/plain, Size: 980 bytes --]
--- a/packages/net/bsd_tcpip/current/ChangeLog
+++ b/packages/net/bsd_tcpip/current/ChangeLog
@@ -1,5 +1,10 @@
+2010-01-29 Cameron Taylor <cameron.taylor@vecima.com>
+
+ * src//sys/kern/sockio.c: free socket for connection closed while
+ on the accept queue.
+
2009-07-22 John Dallaway <john@dallaway.org.uk>
* include/sys/cdefs.h: Silence undefined macro warning reported
by Jay Foster.
--- a/packages/net/bsd_tcpip/current/src/sys/kern/sockio.c
+++ b/packages/net/bsd_tcpip/current/src/sys/kern/sockio.c
@@ -379,10 +379,12 @@
new_fp->f_xops = (CYG_ADDRWORD)&bsd_sockops;
sa = 0;
error = soaccept(so, &sa);
if (error) {
+ /* If the socket cannot be accepted, then it must be closed and freed */
+ soclose(so);
/*
* return a namelen of zero for older code which might
* ignore the return value from accept.
*/
if (name != NULL) {
[-- Attachment #3: fulldiff.txt --]
[-- Type: text/plain, Size: 4781 bytes --]
--- a/packages/net/bsd_tcpip/current/ChangeLog
+++ b/packages/net/bsd_tcpip/current/ChangeLog
@@ -1,5 +1,11 @@
+2010-01-29 Cameron Taylor <cameron.taylor@vecima.com>
+
+ * src//sys/kern/sockio.c: free socket for connection closed while
+ on the accept queue. Also don't leak the error'd socket handle,
+ fix checks for NULL anamelen and error on copyout.
+
2009-07-22 John Dallaway <john@dallaway.org.uk>
* include/sys/cdefs.h: Silence undefined macro warning reported
by Jay Foster.
--- a/packages/net/bsd_tcpip/current/src/sys/kern/sockio.c
+++ b/packages/net/bsd_tcpip/current/src/sys/kern/sockio.c
@@ -288,18 +288,15 @@
static int
bsd_accept(cyg_file *fp, cyg_file *new_fp,
struct sockaddr *name, socklen_t *anamelen)
{
- socklen_t namelen = 0;
+ socklen_t namelen;
int error = 0, s;
struct socket *head, *so;
struct sockaddr *sa;
- if( anamelen != NULL)
- namelen = *anamelen;
-
s = splsoftnet();
head = (struct socket *)fp->f_data;
if ((head->so_options & SO_ACCEPTCONN) == 0) {
splx(s);
@@ -340,108 +337,58 @@
*/
so = TAILQ_FIRST(&head->so_comp);
TAILQ_REMOVE(&head->so_comp, so, so_list);
head->so_qlen--;
-#if 0 // FIXME
- fflag = lfp->f_flag;
- error = falloc(p, &nfp, &fd);
- if (error) {
- /*
- * Probably ran out of file descriptors. Put the
- * unaccepted connection back onto the queue and
- * do another wakeup so some other process might
- * have a chance at it.
- */
- TAILQ_INSERT_HEAD(&head->so_comp, so, so_list);
- head->so_qlen++;
- wakeup_one(&head->so_timeo);
- splx(s);
- goto done;
- }
- fhold(nfp);
- p->p_retval[0] = fd;
-
- /* connection has been removed from the listen queue */
- KNOTE(&head->so_rcv.sb_sel.si_note, 0);
-#endif
-
so->so_state &= ~SS_COMP;
so->so_head = NULL;
cyg_selinit(&so->so_rcv.sb_sel);
cyg_selinit(&so->so_snd.sb_sel);
- new_fp->f_type = DTYPE_SOCKET;
- new_fp->f_flag |= FREAD|FWRITE;
- new_fp->f_offset = 0;
- new_fp->f_ops = &bsd_sock_fileops;
- new_fp->f_data = (CYG_ADDRWORD)so;
- new_fp->f_xops = (CYG_ADDRWORD)&bsd_sockops;
-
- sa = 0;
+ sa = NULL;
error = soaccept(so, &sa);
if (error) {
+ /* If the socket cannot be accepted, then it must be closed and freed */
+ soclose(so);
/*
* return a namelen of zero for older code which might
* ignore the return value from accept.
*/
- if (name != NULL) {
+ if (anamelen != NULL) {
*anamelen = 0;
}
- goto noconnection;
}
- if (sa == NULL) {
- namelen = 0;
- if (name)
- goto gotnoname;
- splx(s);
- error = 0;
- goto done;
- }
- if (name) {
- if (namelen > sa->sa_len)
- namelen = sa->sa_len;
+ else {
+ new_fp->f_type = DTYPE_SOCKET;
+ new_fp->f_flag |= FREAD|FWRITE;
+ new_fp->f_offset = 0;
+ new_fp->f_ops = &bsd_sock_fileops;
+ new_fp->f_data = (CYG_ADDRWORD)so;
+ new_fp->f_xops = (CYG_ADDRWORD)&bsd_sockops;
+
+ if (sa == NULL) {
+ if (anamelen) {
+ *anamelen = 0;
+ }
+ }
+ else if (name != NULL && anamelen != NULL) {
+ namelen = *anamelen;
+
+ if (namelen > sa->sa_len)
+ namelen = sa->sa_len;
#ifdef COMPAT_OLDSOCK
- if (compat)
- ((struct osockaddr *)sa)->sa_family = sa->sa_family;
+ if (compat)
+ ((struct osockaddr *)sa)->sa_family = sa->sa_family;
#endif
- error = copyout(sa, (caddr_t)name, namelen);
- if (!error)
-gotnoname:
- *anamelen = namelen;
- }
-noconnection:
-
-#if 0 // FIXME
- /*
- * close the new descriptor, assuming someone hasn't ripped it
- * out from under us.
- */
- if (error) {
- if (fdp->fd_ofiles[fd] == nfp) {
- fdp->fd_ofiles[fd] = NULL;
- fdrop(nfp, p);
- }
- }
- splx(s);
+ memcpy((caddr_t)name, sa, namelen);
+ }
+ }
- /*
- * Release explicitly held references before returning.
- */
-done:
- if (nfp != NULL)
- fdrop(nfp, p);
- fdrop(lfp, p);
- return (error);
- m_freem(nam);
-#else
- done:
splx(s);
if (sa)
FREE(sa, M_SONAME);
-#endif
return (error);
}
// -------------------------------------------------------------------------
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2010-02-01 20:53 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-01 20:53 bsd_tcpip stack - socket lost when connection is closed while on accept queue Cameron Taylor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).