buglwIP - A Lightweight TCP/IP stack - Bugs: bug #46701, Non-standard close behavior can...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #46701: Non-standard close behavior can result in socket/netconn leaks

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Wed 16 Dec 2015 11:03:32 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Mon 21 Mar 2016 02:39:16 PM UTC, comment #11: 

Moved the issue of blocking during memory failure over to:

bug #47485 (https://savannah.nongnu.org/bugs/index.php?47485)

Joel Cunningham <jcunningham>
Group Member
Fri 18 Mar 2016 06:59:16 PM UTC, comment #10: 

Right.

> If we want to improve the closure process to have no blocking in the default closure
> behavior, maybe that would be better as another task


Yes. I'd be glad if you could file a bug and add a good description :-)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 18 Mar 2016 03:55:54 PM UTC, comment #9: 

Simon,

I'm fine with closing this out for now.  Both the issues my application ran into are accounted for with changes on master.

If we want to improve the closure process to have no blocking in the default closure behavior, maybe that would be better as another task since this bug was targeted at the socket/netconn leaks

Joel Cunningham <jcunningham>
Group Member
Thu 17 Mar 2016 08:13:28 PM UTC, comment #8: 

I'm fine with addressing the RST-on-close by translating the err to ERR_OK and allowing the netconn/socket to be cleaned up appropriately.  Feel free to move that to a separate bug if desired, thanks!

Joel Cunningham <jcunningham>
Group Member
Thu 17 Mar 2016 08:05:41 PM UTC, comment #7: 

I've committed the fix for nonblocking sockets, so only the RST-on-close remains. Although that one might deserve its own bug entry...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 17 Mar 2016 07:52:05 PM UTC, comment #6: 

Reading up on this, you're probably right that lwIP-internal resource shortage should not lead to EWOULDBLOCK on socket layer. I'm OK with changing the netconn API in this respect, too.
I'll change the code to fix this 1st bug. However, there might be

However, deleting the netconn & socket in all 'err != ERR_OK' cases is definitively wrong and can lead to pcb leaks (instead of the netconn/socket leaks you want to fix).

So why don't we solve this 2nd bug by returning ERR_OK when from 'err_tcp()' (which is called on RST) if 'old_state == NETCONN_CLOSE' ? Noone is interested in the RST in this case anyway.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 17 Mar 2016 06:56:13 PM UTC, comment #5: 

Simon,

Here's the appropriate text.  It's located under the section on SO_LINGER (7.5 Generic Socket Options):

"This option specifies how the close function operates for a connection-oriented protocol (e.g., for TCP and SCTP, but not for UDP). By default, close returns immediately, but if there is any data still remaining in the socket send buffer, the system will try to deliver the data to the peer.

The SO_LINGER socket option lets use change this default..."

I agree with you that the ideal behavior would be for LwIP to initiate an asynchronous closure and return caller immediately, even when we can't send a FIN because the send buffer is full.

In my patch, I removed the non-blocking case because my applications are expecting the default close behavior as described by Stevens and won't call close again if it receives EWOULDBLOCK.  It was better to block the caller for a couple of milliseconds for an ACK to come in and the send buffer to open up, allowing LwIP to send a FIN and successfully initiate the closure, rather than leak the socket.

My comment #2 describes a case where during the caller being blocked, trying to initiate a closure (current LwIP behavior), a RST comes in and generates an error.  This causes the blocking close to return, but with an error, so it leaks the netconn and socket.

I don't think my change to free the socket/netconn is too early because once the non-blocking behavior has been disabled, the call to close either completes successfully or times out.  Either case the PCB should be freed by the time we are returning the caller.  This is just a work-around and if the final solution we want to leave those intact during the asynchronous closure, that's fine as long as the responsibility has been removed from the application.

These issues are seen with the default close behavior.  I have not enabled SO_LINGER support in my port, but care should be taken to make sure we continue adhering to standard SO_LINGER behavior.  The winsock documentation has a nice little table describe the closure behaviors (default and SO_LINGER): https://msdn.microsoft.com/en-us/library/windows/desktop/ms737582%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396

Joel Cunningham <jcunningham>
Group Member
Thu 18 Feb 2016 07:09:23 PM UTC, comment #4: 

I can follow you on Stevens, but he doesn't talk about nonblocking sockets or is he? I don't know if we're doing something wrong here or not.

By making nonblocking sockets behave like blocking sockets, you lose the nonblocking thing, so it might be better to let close return and set a flag in the netconn to dealloc on close success (and yes, always dealloc the socket in close()).

I'm not sure I can follow you on comment #2 though.

The patch in comment #3 seems to dealloc socket/netconn too early without freeing the pcb... or am I wrong?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 14 Jan 2016 12:33:20 AM UTC, comment #3: 

I'm attaching some work-arounds for this issue (which I'm using in my port) for anyone to use before Simon gets back and we can work through the proper fix.

(file #36021)

Joel Cunningham <jcunningham>
Group Member
Thu 17 Dec 2015 07:54:23 PM UTC, comment #2: 

I found another case where this is happening.  If once in a blocking close, trying to send a FIN, if an RST is received, the TCP error callback is called, returning ERR_RST (ECONNRESET) to the application thread that is blocked.  When this happens, the netconn and socket are leaked

The area in netconn_delete() only calls netconn_free() if err equals ERR_OK.  The same happens in lwip_close(), free_socket() is only called if err == ERR_OK

This is a slightly different problem than the EWOULDBLOCK as the PCB has indeed been deleted, but then we fail deleting the socket/netconn

I'm not sure if we try to detect if the PCB is gone during error handling in netconn_delete and only delete if so?

Ideally, as outlined below, we would want to delete the netconn/socket in all cases


Joel Cunningham <jcunningham>
Group Member
Wed 16 Dec 2015 11:05:31 PM UTC, comment #1: 

Here's the Steven's text, apologies for messed up formatting:


The default action of close with a TCP socket is to mark the socket
as closed and return to the process immediately.  The socket descriptor
is no longer usable by the process: It cannot be used as an argument to
read or write.  But, TCP will try to send any data that is already queued
to be sent to the other end, and after this occurs, the normal TCP
connection termination sequence takes place.


Joel Cunningham <jcunningham>
Group Member
Wed 16 Dec 2015 11:03:32 PM UTC, original submission:  

I've encountered non-standard close behavior that can result in socket and netconn leaks.  Long explanation here, please bear with me

My application is using non-blocking sockets and when trying to close the socket, EAGAIN is returned because the snd_buf in the PCB is full and we can't send a FIN

Since the socket is set to non-blocking, the close returns without actually closing the socket.  My application, written against standard BSD sockets, isn't checking for EAGAIN as that would not normally be returned from close unless SO_LINGER was set and the timeout elapsed.  Even if it was, it doesn't mean the socket is still allocated.  With this behavior, non-blocking applications need a wait loop around close until it returns successfully.

I double checked the BSD behavior in my Steven's book (UNIX Network Programming Volume 1, 3rd Edition: The Sockets Networking API) and it documents (section 4.9):


The default action of close with a TCP socket is to mark the socket as closed and return to the process immediately.  The socket descriptor is no longer usable by the process: It cannot be used as an argument to read or write.  But, TCP will try to send any data that is already queued to be sent to the other end, and after this occurs, the normal TCP connection termination sequence takes place.


The section on SO_LINGER explains returning EWOULDBLOCK during a SO_LINGER timeout and I can provide that text if it is helpful.

So I see the following deviations:

  1. close in the default case of SO_LINGER off shouldn't be blocking regardless of blocking/non-blocking socket configuration.  The closure happens asynchronously
  2. close shouldn't ever leave socket resources allocated
  3. close should only be returning EWOULDBLOCK in the case of a SO_LINGER timeout (or using non-blocking sockets with SO_LINGER configured with a timeout)


I also found this commit in lwip_netconn_do_close_internal, indicating the deviation #1 is known:


f(err == ERR_MEM) {
      /* Closing failed because of memory shortage */
      if (netconn_is_nonblocking(conn)) {
        /* Nonblocking close failed */
        close_finished = 1;
        err = ERR_WOULDBLOCK;
      } else {
        /* Blocking close, check the timeout */
#if LWIP_SO_SNDTIMEO || LWIP_SO_LINGER
        s32_t close_timeout = LWIP_TCP_CLOSE_TIMEOUT_MS_DEFAULT;
        /* this is kind of an lwip addition to the standard sockets: we wait
           for some time when failing to allocate a segment for the FIN */
#if LWIP_SO_SNDTIMEO
        if (conn->send_timeout >= 0) {
          close_timeout = conn->send_timeout;
        }


I'm thinking why behave differently for nonblocking sockets since we aren't supposed to be blocking for either case?  Shorter term fix might just to remove the check netconn_is_nonblocking() and this would make sure close can complete the FIN up till the 20 second timeout.

Longer term, we could change over to having the closure complete asynchronously and having close always deallocate resources.  Thoughts?

Joel Cunningham <jcunningham>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jcunningham (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-03-18 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-03-17 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2016-01-14 jcunningham Attached File- Added 0001-Work-around-for-bug-46701-close-leaks-sockets.patch, #36021

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code