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
|