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 authentification level.

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

Submitted by:  Berend Ozceri <berend>
Submitted on:  Wed Nov 14 21:24:35 2007  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: None

Mon Nov 26 19:09:09 2007, comment #1:

Fixed.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed Nov 14 21:24:35 2007, 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>

 

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 berend (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 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon Nov 26 19:09:09 2007goldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1