buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27215, TCP sent() callback gives leagin...

 
 

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

bug #27215: TCP sent() callback gives leagin and trailing 1 byte len (SYN/FIN)

Submitter:  Chris N. Strahm <cstrahm>
Submitted:  Mon 10 Aug 2009 01:34:20 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Feature Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

Jump to the original submission

Wed 21 Oct 2009 03:45:23 PM UTC, comment #7: 

Fixed again by
a) for SYN, decreasing pcb->acked by one in state SYN_RCVD and
b) for FIN, decreasing pcb->acked by one when removing a segment containing FIN from unsent or unacked

Simon Goldschmidt <goldsimon>
Group administrator
Wed 21 Oct 2009 03:17:35 PM UTC, comment #6: 

D'oh, didn't think of that. Your objection is of course correct.

I'll have a thought about your proposed solution.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 21 Oct 2009 11:37:24 AM UTC, comment #5: 

I think I have an objection!  Sorry for the delay.  I don't quite understand the new "if ()" block.

The code needs to exclude a single byte from pcb->acked when the sequence acked contains a SYN or a FIN.  With the code as checked in, if pcb->acked is > 1, but contains a SYN or FIN, we'll still send the event including the extra byte.  This might not be a problem with the SYN, but I think we could easily get an ACK for the FIN that also acked some data, and so pcb->acked > 1. 

Rather than relying on the current state and the number of bytes being acked, I think we need to check the sequence being acked to see if that sequence contains the SYN or FIN.  This could probably be most efficiently done by decrementing pcb->acked when we pull a packet off the unacked or unsent list that has a SYN or FIN in it.

Kieran Mansley <kieranm>
Group Member
Sun 18 Oct 2009 08:31:00 AM UTC, comment #4: 

Checked in as there were no objections...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 16 Oct 2009 02:31:32 PM UTC, comment #3: 

The attached patch tries to solve this bug by using this condition:

if ((pcb->acked != 1) || ((old_state != SYN_RCVD) &&
    (pcb->state != FIN_WAIT_2) && (pcb->state != TIME_WAIT))) {
  TCP_EVENT_SENT(pcb, pcb->acked, err);
}

thus not calling the sent callback if one byte was acked in a specific state (or state before calling tcp_process).

Tested with the httpd and a raw api tcp client.

Any objections?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 25 Aug 2009 09:30:43 AM UTC, comment #2: 

Updated the summary to better describe the problem

Simon Goldschmidt <goldsimon>
Group administrator
Sun 23 Aug 2009 12:30:32 PM UTC, comment #1: 

I have just been debugging this and the 1 byte is of course the FIN being acked. The same problem exists with the initial SYN, which is because the http_sent function always gets called with 2 bytes more than the file size (unless tcp_sent is called to reset the callback too early, of course).

Since at the point the sent function is called (in tcp_input), we could only rely on the pcb's state, I guess (in case of SYN received, the state before the syn is actually processed).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 10 Aug 2009 01:34:20 PM UTC, original submission:  


> What does the 1 byte length mean?


It's called, I'm guessing, when the other side processes the FIN.  The sent callback is called as a result of data being ACKed, which means that buffer space should be available to send.  It just so happens in this case that the sequence being acked is the FIN to mark the end of the connection.  This could probably be improved, as I can see it's confusing and not useful.  Feel free to file a feature request bug on
savannah.

> Why does close get called twice?


I'm guessing that you're using code based on the httpserver_raw example. If you look at that source, close_conn() is called for a number of reasons.  The first is from the http_sent() callback if there's no data left, so almost certainly being triggered by the behaviour outlined above.  The second is when it receives a packet and it's either not a GET request, or it's a NULL pbuf.  You're almost certainly seeing a NULL pbuf passed to the receive callback which means that the other end has closed the connection.  This is normal, as you have finished the HTTP
transaction - it's just the way that the other end closing a connection is signalled through the API.

Chris N. Strahm <cstrahm>

 

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

No files currently attached

 

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 kieranm (Updated the item)
  • -email is unavailable- added by cstrahm (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-10-21 goldsimon Open/ClosedOpen Closed
        StatusIn Progress Fixed
    2009-10-21 kieranm Open/ClosedClosed Open
        StatusFixed In Progress
    2009-10-18 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2009-10-16 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2009-10-15 kieranm Planned Release1.4.0 1.3.2
    2009-08-25 goldsimon Summaryhttp_sent() gives trailing 1 byte len [raw] TCP sent() callback gives leagin and trailing 1 byte len (SYN/FIN)
    2009-08-20 kieranm Planned Release 1.4.0

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code