patchlwIP - A Lightweight TCP/IP stack - Patches: patch #10013, tcp_in: Flag the pcb as closing if...

 
 

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

patch #10013: tcp_in: Flag the pcb as closing if TCP_EVENT_CLOSED refused

Submitter:  David Cermak <david_cermak>
Submitted:  Thu 07 Jan 2021 06:45:45 PM UTC
   
 
Category:  TCP Priority:  5 - Normal
Status:  In Progress Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Open
Planned Release:  None

Thu 18 Mar 2021 05:20:06 PM UTC, comment #3: 

Small update of the patch: Added error handling of the `TCP_EVENT_CLOSED() inside tcp_process_refused_data(), as it could fail here, as well, for the same reason.

tcp-TF_REFUSED_DATA-flag-for-event-refused_v2.patch

(file #51085)

David Cermak <david_cermak>
Wed 10 Mar 2021 05:44:43 PM UTC, comment #2: 

Thanks for the feedback. Yes, I see. I just tried to have the SM  recover from this state using a minimal changeset, but was in fact fixing the symptoms, not the bug.

PTAL at the reworked patch. Added a new "pcb->flag: TF_REFUSED_DATA", if set, it means that some `TCP_EVENT_...()` failed (if some data associated with the event, "pcb->refused_data" still holds the pbuf).

I've decided against extending the `refused_data` field with enum values for TCP_EVENT_CLOSED, TCP_EVENT_RECV, since it would make the `pcb` struct bigger, or needed an extra allocation. Could have also allocated an empty pbuf to hold the flags (as it's done for the FIN with data), but still an extra allocation (even in this unlikely case). I think that using the `(pcb->flags & TF_REFUSED_DATA)` is the most compact solution, and more or less clean, too.

Now we have these three situations:
1) TCP_EVENT_RECV only: Post the refused_data next time -- no change
2) both TCP_EVENT_CLOSED and TCP_EVENT_RECV: Post the refused data first, then the closed event -- no change
3) TCP_EVENT_CLOSED only: Post the null/EOF event (TCP_EVENT_RECV(NULL) is equivalent -- newly added
Still need to differentiate between 2 and 3, since for 2 we have to postpone the data event to come before the EOF event



(file #51036)

David Cermak <david_cermak>
Thu 04 Mar 2021 08:39:39 PM UTC, comment #1: 

I'm glad you found this bug, but I'm not sure this is the correct fix. It seems you're mixing directions here: receiving a FIN does not mean sending one. But setting TF_CLOSEPEND essentially means "send a FIN next time you check this pcb".

However, a FIN should only be sent if the application gracefully closes the TCP connection. If you wanted to do something in the stack, a RST would be better. But since we don't want to break anything here, why don't we reuse "pcb->refused_data" for this? Using NULL here won't work, so we need a special value for TCP_EVENT_CLOSED vs. TCP_EVENT_RECV, but I think that might work better.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 07 Jan 2021 06:45:45 PM UTC, original submission:  

This fixes issue which has been reported


If the packet contained FIN flag, we post a TCP_EVENT_CLOSE with null pbuf, which could get lost if the underlying platform implementation of sys_mbox_(try)post() returns ERR_MEM (i.e. won't fit into the recv mailbox).
Loosing this event causes trouble since the FIN initiator gets ACK'ed and assumes the connection has closed, but the TCP state machine is stuck in its active state.

Fixed by flagging the pcb as closing if the ERR_MEM returned.

This could be perhaps fixed by adding LWIP_EVENT_API and implement the events in the port layers, but think this way it is cleaner. Open to suggestions.

David Cermak <david_cermak>

 

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

 

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 david_cermak (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
    2021-03-18 david_cermak Attached File- Added tcp-TF_REFUSED_DATA-flag-for-event-refused_v2.patch, #51085
    2021-03-10 david_cermak Attached File- Added tcp-Use-TF_REFUSED_DATA-flag-to-indicate-pcb-refused.patch, #51036
    2021-03-04 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2021-01-07 david_cermak Attached File- Added tcp_in-Flag-the-pcb-as-closing-if-TCP_EVENT_CLOSED-refused.patch, #50674

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code