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()

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Wed 18 Jan 2017 10:18:35 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Invalid
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  2.0.2
lwIP version:  git head

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>
Group administrator
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>
Group Member
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>
Group Member
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>
Group administrator
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>
Group Member
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>
Group administrator
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>
Group Member
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>
Group administrator
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>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by axellin (Updated the item)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-02-08 goldsimon StatusNone Invalid
        Open/ClosedOpen Closed
    2017-01-18 goldsimon StatusIn Progress None
    2017-01-18 axellin Attached File- Added 0001-tcp_out-Don-t-reset-pcb-unsent_oversize-when-requeue.patch, #39500

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code