buglwIP - A Lightweight TCP/IP stack - Bugs: bug #50075, TCP_OVERSIZE fails on...

 
 

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

bug #50075: TCP_OVERSIZE fails on tcp_rexmit_rto()

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Wed 18 Jan 2017 10:18:35 AM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Invalid
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: 2.0.2
lwIP version: git head

(Jump to the original submission Jump to the original submission)

Wed 08 Feb 2017 09:06:39 PM UTC, comment #8:

You have not hit that assert because since somewhere in 2011, we set oversize_left to 0 for every segment sent (via tcp_output_segment). Still I think it's more correct to leave it like it is (if the previous statement shouldn't hold in the future).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 31 Jan 2017 05:06:04 AM UTC, comment #7:

Regarding comment #5,
I think you are right, my changes for tcp_rexmit() is wrong.
When add the first unacked segment to the tail of non-empty unsent queue, it needs to reset pcb->unsent_oversize.
i.e. I think just revert patch #8237 is fine, I never hit that assertion.

Axel Lin <axellin>
Tue 24 Jan 2017 09:46:23 AM UTC, comment #6:

I have tested adding debug code to check if it possible pcb->unsent_oversize !=0 when pcb->unsent is NULL in these places. And it never happened.
I didn't hit the assertion as mentioned in patch #8237.

Axel Lin <axellin>
Tue 24 Jan 2017 09:41:27 AM UTC, comment #5:

OK, thanks for explaining the tcp_rexmit_rto() part. However, see patch #8237 why that code is necessary. We'd have to reset "seg->oversize_left" of that segment to get rid of this code or an assertion matching "seg->oversize_left" vs "pcb->unsent_oversize" fails.

I still don't understand the non-rto rexmit change, nor do I understand why that code is like it is right now ;-)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 18 Jan 2017 08:54:15 PM UTC, comment #4:

seg->oversize_left is set to 0 before adding seg to unacked list.
so
#if TCP_OVERSIZE_DBGCHECK
/* if last unsent changed, we need to update unsent_oversize */
if (pcb->unsent == NULL) {
pcb->unsent_oversize = seg->oversize_left;
}
#endif /* TCP_OVERSIZE_DBGCHECK */

is always equivalient to

#if TCP_OVERSIZE_DBGCHECK
/* if last unsent changed, we need to update unsent_oversize */
if (pcb->unsent == NULL) {
pcb->unsent_oversize = 0;
}
#endif /* TCP_OVERSIZE_DBGCHECK */

However, if pcb->unsent is NULL before adding unacked entries, pcb->unsent_oversize should be 0. (otherwise, it's a bug).
So it's safe to do nothing here.
(Actually, I think reset pcb->unsent_oversize = 0 here might hide bug.

Note, I found the orignial code looks strange when review the code.
I'm checking some issue, but in the end my test shows it's nothing to do with this part.

Axel Lin <axellin>
Wed 18 Jan 2017 08:43:31 PM UTC, comment #3:

I don't get it, shouldn't we reset unsent_oversized to zero in this case instead of doing nothing? You said you had a problem with this being disable when DBGCHECK is disabled and now you want to completely remove the code??

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 18 Jan 2017 11:35:15 AM UTC, comment #2:

Hi Simon,
I add a patch for review as I think the reset pcb->unsent_oversize is not necessary at these places. When pcb->unsent is NULL before adding unacked entries, pcb->unsent_oversize is supposed to be 0 already.

(file #39500)

Axel Lin <axellin>
Wed 18 Jan 2017 10:23:44 AM UTC, comment #1:

Proposed fix: set pcb->unsent_oversize to zero for "TCP_OVERSIZE && !TCP_OVERSIZE_DBGCHECK" in this case.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 18 Jan 2017 10:18:35 AM UTC, original submission:

When TCP_OVERSIZE_DBGCHECK is disabled, pcb->unsent_oversize might not be updated in tcp_rexmit_rto().

This bug has always been there since the TCP_OVERSIZE code has been added. Patch #8237 trying to fix this in Jan 2014 fixed it only for the DBGCHECK case.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.

 

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by axellin (Updated the item)
  • -unavailable- added by goldsimon (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 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 08 Feb 2017 09:06:39 PM UTCgoldsimonStatusNone=>Invalid
      Open/ClosedOpen=>Closed
    Wed 18 Jan 2017 08:43:31 PM UTCgoldsimonStatusIn Progress=>None
    Wed 18 Jan 2017 11:35:15 AM UTCaxellinAttached File-=>Added 0001-tcp_out-Don-t-reset-pcb-unsent_oversize-when-requeue.patch, #39500

    Back to the top


    Powered by Savane 3.1-cleanup1