[lwip-devel] [bug #21582] pcb->acked accounting can be wrong when ACKs arrive out-of-order

[Berend Ozceri]

URL:
  <http://savannah.nongnu.org/bugs/?21582>

                 Summary: pcb->acked accounting can be wrong when ACKs arrive
out-of-order
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: berend
            Submitted on: Wednesday 11/14/2007 at 21:24
                Category: TCP
                Severity: 3 - Normal
              Item Group: Faulty Behaviour
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: 

    _______________________________________________________

Details:

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





    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?21582>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/