buglwIP - A Lightweight TCP/IP stack - Bugs: bug #50276, tcp: fix accept event on closed...

 
 

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

bug #50276: tcp: fix accept event on closed listening PCBs

Submitter:  David van Moolenbroek <dcvmoole>
Submitted:  Fri 10 Feb 2017 05:48:22 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Tue 28 Feb 2017 07:28:24 PM UTC, comment #6: 

As you can see from the commits today, using the event api is limited to very specific use cases anyway where you have only one "application" sitting on top of lwIP: I had to disable all TCP applications for the !callback-api case...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 28 Feb 2017 05:45:58 PM UTC, comment #5: 

Neat, thanks!

> Oh, and I think you should stay with the event API :-) Please ;-)


Well, after a bunch of subsequent changes, switching back is not entirely trivial anymore, but it is on my TODO list now.

David van Moolenbroek <dcvmoole>
Tue 28 Feb 2017 11:14:31 AM UTC, comment #4: 

I've added bug #50418 for the new bug pointed out in comment #2. Should be already fixed.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 28 Feb 2017 08:08:22 AM UTC, comment #3: 

Well spotted! And just in time for 2.0.2 :-)
I'd vote for solution 1) but just check state==SYN_RCVD (listener is only available for TCP_LISTEN_BACKLOG with the event API). That way, the EVENT_API gets more like the CALLBACK_API where an application doesn't know about the new pcb before the accept callback.
BTW, there is a theoretical problem of the same kind in FIN_WAIT_1 state if SYN_RCVD is closed, but since we don't pass a SYN_RCVD pcb to the application, there's no way to close it. And in all situations where the stack internally closes such a pcb, a RST is sent (so not resulting in FIN_WAIT_1).
Oh, and I think you should stay with the event API :-) Please ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 27 Feb 2017 10:39:44 PM UTC, comment #2: 

Hmm well, even though I stand by this patch and I still believe it is an improvement, I feel obliged to add the following. The same crash happened again, and this time I managed to reproduce it. As it turns out, the reason is that with the events API, events other than ACCEPT are also fired on connections that are arriving, but have not yet been accepted, on a listening socket. That is, connections in SYN_RCVD state. Such connections may see not only ACCEPT events, but also POLL and ERR events. Those events are obviously not interesting to the higher layer, but unlike with the callback API, where no callback functions will have been set for them yet, the event API will pass them to the event handler anyway.

Since the PCBs of such connections will have the callback_arg copied from the listening PCB, and this argument is (I assume) typically used to point to a socket structure with additional data, this poses a major problem: if there is a chance that the listening socket has been closed already (simply due to an application close() call or so), then the callback_arg of the arriving PCB will effectively be a dangling pointer. For POLL events on the SYN_RCVD connection, this is not a problem, because in that case, the event handler can check whether the PCB's state is SYN_RCVD and the listener field is NULL (meaning the listening socket has disappeared), and simply disregard the event in that case. For ERR events on the SYN_RCVD connection however, the PCB is already gone, and all the event handler has to go on, is a dangling pointer to what used to be the listening PCB.

This is the current situation, and as I see it, there are only two possible ways forward:
1) add checks for state==SYN_RCVD && listener==NULL to the POLL and ERR event triggers in the lwIP core TCP code to make sure such events are not passed to the event handler in that case, or,
2) deem the entire event API suitable only for use cases where listening sockets, if any, are never closed, i.e. only for very basic uses of lwIP.

I'm not sure what is the best option here. I think #1 might be helpful for all use cases, including the "basic" use, but I think #2 would be fine as well. Either way, I caved and switched over my own implementation to the callback API after all, just to be on the safe side at least for now.

David van Moolenbroek <dcvmoole>
Fri 10 Feb 2017 08:32:06 PM UTC, comment #1: 

Right. Thanks for the patch. EVENT_API that is. Good to have someone testing it in that direction...

We really need someone to work on task #14097 :-(

Simon Goldschmidt <goldsimon>
Group administrator
Fri 10 Feb 2017 05:48:22 PM UTC, original submission:  

I just got a rare crash I cannot reproduce, but all the debug information points to my side of things trying to process a TCP accept event for a new PCB for which the listening PCB was already gone.

Within lwIP, listener tracking is in place if either LWIP_CALLBACK_API or TCP_LISTEN_BACKLOG is defined, except that there is exactly one place where the listener is checked only if LWIP_CALLBACK_API is defined, and not if TCP_LISTEN_BACKLOG is defined. And that place prevents closed listening PCBs from triggering accept events. And indeed, my config defines TCP_LISTEN_BACKLOG but not LWIP_CALLBACK_API.

Attached patch should fix this problem.

===

If LWIP_CALLBACK_API is not defined, but TCP_LISTEN_BACKLOG is, then the LWIP_EVENT_ACCEPT TCP event may be triggered for closed listening sockets. This case is just as disastrous for the event API as it is for the callback API, as there is no way for the event hook to tell whether the listening PCB is still around. Add the same protection against this case for TCP_LISTEN_BACKLOG as was already in place for LWIP_CALLBACK_API.

Also remove one NULL check for LWIP_CALLBACK_API that had already become redundant for all callers, making the TCP_EVENT_ACCEPT code for that callback wrapper more in line with the rest of the wrappers.

David van Moolenbroek <dcvmoole>

 

(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 dcvmoole (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-02-10 goldsimon StatusNone Fixed
        Open/ClosedOpen Closed
    2017-02-10 dcvmoole Attached File- Added 0001-tcp-fix-accept-event-on-closed-listening-PCBs.patch, #39722

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code