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 ;-)
|
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.
|
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.
|