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)

Submitted by:  Chris N. Strahm <cstrahm>
Submitted on:  Mon 10 Aug 2009 01:34:20 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Feature RequestStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.3.0

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
Sun 18 Oct 2009 08:31:00 AM UTC, comment #4:

Checked in as there were no objections...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
Tue 25 Aug 2009 09:30:43 AM UTC, comment #2:

Updated the summary to better describe the problem

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by kieranm (Updated the item)
  • -unavailable- added by cstrahm (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 21 Oct 2009 03:45:23 PM UTCgoldsimonOpen/ClosedOpen=>Closed
      StatusIn Progress=>Fixed
    Wed 21 Oct 2009 11:37:24 AM UTCkieranmOpen/ClosedClosed=>Open
      StatusFixed=>In Progress
    Sun 18 Oct 2009 08:31:00 AM UTCgoldsimonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Fri 16 Oct 2009 02:31:32 PM UTCgoldsimonStatusNone=>In Progress
      Assigned toNone=>goldsimon
    Thu 15 Oct 2009 03:30:24 PM UTCkieranmPlanned Release1.4.0=>1.3.2
    Tue 25 Aug 2009 09:30:43 AM UTCgoldsimonSummaryhttp_sent() gives trailing 1 byte len [raw]=>TCP sent() callback gives leagin and trailing 1 byte len (SYN/FIN)
    Thu 20 Aug 2009 02:11:49 PM UTCkieranmPlanned Release=>1.4.0

    Back to the top


    Powered by Savane 3.1-cleanup1