Re: [lwip-devel] tcp_receive() possible ancient bug

[Joel Cunningham]

The organization looks incorrect to me, but it may not actually result in any 
wrong behavior, because the loop going through the unsent queue is checking the 
ACK against the packet on the queue.  For the snd_buf update, I think 
recv_acked would just be 0.  So it results in wasted work.

I noticed this because for task #14128 (ABC support) in order to handle the RTO 
case, I’m having to do some extra checking of when retransmitted segments are 
moved from unsent (unacked queued is moved to unsent during RTO).  So I didn’t 
want to add MORE code after the unsent queue check without first correcting 
this.

I’d be willing to push a patch moving the code to within the conditional.

Joel

> On Mar 22, 2017, at 1:48 PM, address@hidden wrote:
> 
> Hmm, I've never questioned that part of the stack :-)
> It might be wrong or just correct by coincidence (i.e. could be written 
> clearer). In any case, it's worth a bug entry to not get forgotten.
> 
> Simon
> 
> 
> Am 21.03.2017 um 23:20 schrieb Joel Cunningham:
>> So I was studying the section of tcp_receive() that handles processing an 
>> ACK for new data.  What I found strange is that there is logic to check the 
>> unsent queue and update pcb->snd_buf outside from the check of whether the 
>> ACK acknowledge new data.  I was expecting this logic to be within the 
>> conditional that checks ackno is between lastack+1 and snd_nxt
>> 
>> } else if (TCP_SEQ_BETWEEN(ackno, pcb->lastack+1, pcb->snd_nxt)) {
>>   /* We come here when the ACK acknowledges new data. */
>> …
>> } else {
>>   /* Out of sequence ACK, didn't really ack anything */
>>   tcp_send_empty_ack(pcb);
>> }
>> 
>> /* We go through the ->unsent list to see if any of the segments
>>     on the list are acknowledged by the ACK. This may seem
>>     strange since an "unsent" segment shouldn't be acked. The
>>     rationale is that lwIP puts all outstanding segments on the
>>     ->unsent list after a retransmission, so these segments may
>>     in fact have been sent once. */
>> ...
>> }
>> pcb->snd_buf += recv_acked;
>> /* End of ACK for new data processing. */
>> 
>> I dug through the history and this organization goes all the way back to the 
>> original commit in git.  Is this a bug or is there a reason to run through 
>> the logic outside of the conditional (which checks for an ACK covering new 
>> data)?
>> 
>> Thanks,
>> 
>> Joel
>> _______________________________________________
>> lwip-devel mailing list
>> address@hidden
>> https://lists.nongnu.org/mailman/listinfo/lwip-devel
> 
> 
> _______________________________________________
> lwip-devel mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/lwip-devel