buglwIP - A Lightweight TCP/IP stack - Bugs: bug #21582, pcb->acked accounting can be...

 
 

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

bug #21582: pcb->acked accounting can be wrong when ACKs arrive out-of-order

Submitted by:  Berend Ozceri <berend>
Submitted on:  Wed 14 Nov 2007 09:24:35 PM UTC
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  * None
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       No canned response available

 

Mon 26 Nov 2007 07:09:09 PM UTC, comment #1: 

Fixed.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 14 Nov 2007 09:24:35 PM UTC, original submission:  

The acked field of every TCP pcb structure is supposed to be set to the number of new bytes that were ACKed by the remote receiver during the tcp_receive() function call for incoming TCP packet processing.

The relevant portion of the tcp_receive() code looks something like:

    if (pcb->lastack == ackno) {
      pcb->acked = 0;
      ...
      Do duplicate ACK processing (initiate fast retransmit, etc.)
      ...
    } else if(TCP_SEQ_BETWEEN(ackno, pcb->lastack+1, pcb->snd_max)){
      ...
      Handle ACK of data that falls between the last ACKed byte and the end of the send window
      ...
      pcb->acked = ackno - pcb->lastack;
      ...
    }

Once tcp_receive() returns back to tcp_process(), the following code is executed to invoke the "sent data has been ACKed" callback for the connection, if one is registered:

    if (pcb->acked > 0) {
      TCP_EVENT_SENT(pcb, pcb->acked, err);
    }

The problem is that tcp_receive() does not handle the case where neither of the two if branches are taken - a case where an ACK arrives for data that's already been ACKed by virtue of a later ACK arriving before the one ACKing earlier data. In that case neither pcb->lastack == ackno, nor TCP_SEQ_BETWEEN(ackno, pcb->lastack+1, pcb->snd_max) is true, so pcb->acked simply holds its old value, which may be non-zero. That can result in tcp_process() mistakenly invoking the callback. While applications that depend solely on the event-oriented aspect of the callback may not be affected (other than being notified spuriously), applications that actually track buffer usage with the call-back length argument can potentially double-count ACKed buffer segments.

The fix is the simple addition of:

    else {
      pcb->acked = 0;
    }

to zero the ACK count in case an out-of-order ACK for already ACKed data arrives.

The reason this bug probably does not matter in typical lwIP usage is that most uses of this callback simply use its event notification aspect, but not the actual ACKed byte counting aspect for fine-tuned buffer management. The callback is typically used to check the new value of pcb->snd_buf, to see if there's send space available, which is accounted for correctly despite the above bug.

Berend

Berend Ozceri <berend>

 

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

Attach Files:
   
   
Comment:
   

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 berend (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.

    Only logged-in users can vote.

     

     

     

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-11-26 goldsimon StatusNone => Fixed
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed

    Back to the top


    Powered by Savane 3.9