buglwIP - A Lightweight TCP/IP stack - Bugs: bug #28054, Two segments with FIN flag on the...

 
 

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

bug #28054: Two segments with FIN flag on the out-of-sequence queue

Submitted by:  Oleg Tyshev <olegreen>
Submitted on:  Thu 19 Nov 2009 04:55:13 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

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

Sun 29 Nov 2009 01:25:09 PM UTC, comment #17:

I reviewed the patch and adapted the out-of-sequence unit tests to cover nearly all the out-of-sequence code.

After these are working successfully (found another bug there where segments were freed but pbufs weren't), I've checked in the patch.

Kieran, I hope you don't mind me taking over here (because of comment #14) :-)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 27 Nov 2009 08:05:25 AM UTC, comment #16:

The unit tests I just checked in seem to confirm the patch works (although not all clauses of the code addin ooseq segments are covered, yet).

I would convert the new function to static though, to give the compiler room for improvements.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 23 Nov 2009 05:40:45 PM UTC, comment #15:

Patch again corrected.
It was error in processing last segment.

if (next->next == NULL &&
TCP_SEQ_GT(seqno, next->tcphdr->seqno)) {
+ if (TCPH_FLAGS(next->tcphdr) & TCP_FIN) {
+ /* segment "next" already contains all data */
+ break;
+ }

(file #19118)

Oleg Tyshev <olegreen>
Mon 23 Nov 2009 03:11:15 PM UTC, comment #14:

I'll take a look at the patch, and if I think it's safe I'll try and get it in to 1.3.2

Kieran Mansley <kieranm>
Project Administrator
Mon 23 Nov 2009 03:08:03 PM UTC, comment #13:

The patch looks good to me. I also like the separation into smaller functions, it would do the rest of the code good, too, since it gets more readable. And at least MSVC and gcc seem to inline static functions quite well, which means that code size isn't an argument here.

However, I'm not in a position to test it and a "should work" might not be enough at this stage of release, so we might want to delay this until after the 1.3.2 release...?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 23 Nov 2009 01:48:03 PM UTC, comment #12:

In this patch common part for insertion packet into
oos queue is moved into separate function.

I have tested this patch with FIN packets.
With other packets it should work too.

(file #19117)

Oleg Tyshev <olegreen>
Fri 20 Nov 2009 06:27:45 PM UTC, comment #11:

There are 4 almost the same come fragments:
1) for ackno ==
2) prev == NULL
3) prev != NULL
4) next->next == NULL
I'll try to simplify it.

Oleg Tyshev <olegreen>
Fri 20 Nov 2009 05:55:14 PM UTC, comment #10:

:)Patch

(file #19095)

Oleg Tyshev <olegreen>
Fri 20 Nov 2009 05:54:35 PM UTC, comment #9:

Sorry, I've made a lot of errors in deallocation.
There is corrected patch.
I will test it, but with slightly changed sources.
(I have other code rules)

Oleg Tyshev <olegreen>
Fri 20 Nov 2009 05:05:00 PM UTC, comment #8:

Selected lines do this work
If first segment overlap completely 2nd, 3rd, etc.,
they will be deleted.

> /*if (TCP_SEQ_LT(prev->tcphdr->seqno, seqno) &&
> TCP_SEQ_LT(seqno, next->tcphdr->seqno)) {*/
> if(TCP_SEQ_BETWEEN(seqno, prev->tcphdr->seqno+1, next->tcphdr->seqno-1)){
> /* The sequence number of the incoming segment is in
> between the sequence numbers of the previous and
> the next segment on ->ooseq. We trim and insert the
> incoming segment and trim the previous segment, if
> needed. */
> cseg = tcp_seg_copy(&inseg);
> if (cseg != NULL) {
> if (TCP_SEQ_GT(prev->tcphdr->seqno + prev->len, seqno)) {
> /* We need to trim the prev segment. */
> prev->len = (u16_t)(seqno - prev->tcphdr->seqno);
> pbuf_realloc(prev->p, prev->len);
> }
> prev->next = cseg;
> if (TCPH_FLAGS(cseg->tcphdr) & TCP_FIN) {
> cseg->next = NULL;
> while (next != NULL) {
> struct tcp_seg *old_seg = next;
> next = next->next;
> memp_free(MEMP_TCP_SEG, old_seg);
> }
> }
> else {
>>>>> /* delete some segments */
>>>>> while (next &&
> TCP_SEQ_GT((seqno + cseg->len),
> (next->tcphdr->seqno + next->len))) {
> struct tcp_seg *old_seg = next;
> next = next->next;
> memp_free(MEMP_TCP_SEG, old_seg);
> }
> if (next &&
> TCP_SEQ_GT(seqno + cseg->len, next->tcphdr->seqno)) {
> /* We need to trim the incoming segment. */
> cseg->len = (u16_t)(next->tcphdr->seqno - seqno);
> pbuf_realloc(cseg->p, cseg->len);
> }
> cseg->next = next;
> }
> }
> break;

Oleg Tyshev <olegreen>
Fri 20 Nov 2009 04:59:34 PM UTC, comment #7:

I'm guessing Oleg can test, and as it's clear that we've got a bug in this situation we can only make it better!

Kieran Mansley <kieranm>
Project Administrator
Fri 20 Nov 2009 04:50:43 PM UTC, comment #6:

> In this patch I have implemented exactly what you have suggested.


For the FIN case, yes, but not for the case when both segments don't contain a FIN. In this case we could still save memory by using only the first segment instead of trimming the first and keeping both segments. This is a rare case though, and probably not worth messing with the code.

Another question: how do we test such things? We would need a test environment that creates such segments, which would bring us to task #7930: Create automatic test cases...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 20 Nov 2009 04:41:08 PM UTC, comment #5:

If we add to the oos-queue packet with FIN flag,
all packets after it can be dropped.

In this patch I have implemented exactly what you have suggested.

Oleg Tyshev <olegreen>
Fri 20 Nov 2009 04:23:15 PM UTC, comment #4:

Why does the patch test for FIN? I think this problem would also be solved if we would always try to remove a second segment instead of trimming the first (if the last seqno is the same).

I don't have a patch for that, though.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 20 Nov 2009 04:12:04 PM UTC, comment #3:

No, we shouldn't remove FIN flag.
It is better to delete some segments.
You could try this patch, but it should be checked.

(file #19092)

Oleg Tyshev <olegreen>
Fri 20 Nov 2009 03:42:25 PM UTC, comment #2:

From looking at the code it looks like we do the right thing when the second segment is in order - there is lots of checking of TCP flags there - but ignore them completely when trimming if the second segment is also out of order. Whenever we trim a segment in that section (tcp_in.c:1244 -> 1363) we need to check the flags and removing any FIN if present.

Kieran Mansley <kieranm>
Project Administrator
Fri 20 Nov 2009 03:36:30 PM UTC, comment #1:

If anyone can propose a simple fix for this in the next couple of days this could go into 1.3.2

Kieran Mansley <kieranm>
Project Administrator
Thu 19 Nov 2009 04:55:13 PM UTC, original submission:

Suppouse we have on the oos_queue one segment
seqno=2100, len=100, FIN flag
After retransmit we receive out of sequence 2nd segment
seqno=1100, len=1100, FIN flag

After trimming we have on the oos queue
two segments with FIN flag
seqno=1100, len=1000, FIN flag
seqno=2100, len=100, FIN flag

If second segment will be received in-sequence - everything all right, oos queue will be deleted,
but in this case flags are not processed correct.

Oleg Tyshev <olegreen>

 

Attached Files
file #19118:  oos_fin.patch added by olegreen (6KiB - text/x-patch)
file #19117:  oos_fin.patch added by olegreen (6KiB - text/x-patch)
file #19095:  oos_fin.patch added by olegreen (6KiB - text/x-patch)
file #19092:  oos_fin.patch added by olegreen (5KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by olegreen (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 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 29 Nov 2009 01:25:09 PM UTCgoldsimonOpen/ClosedOpen=>Closed
    Sun 29 Nov 2009 01:25:08 PM UTCgoldsimonAssigned toNone=>goldsimon
      StatusNone=>Fixed
    Mon 23 Nov 2009 05:40:45 PM UTColegreenAttached File-=>Added oos_fin.patch, #19118
    Mon 23 Nov 2009 01:48:03 PM UTColegreenAttached File-=>Added oos_fin.patch, #19117
    Fri 20 Nov 2009 05:55:14 PM UTColegreenAttached File-=>Added oos_fin.patch, #19095
    Fri 20 Nov 2009 04:12:04 PM UTColegreenAttached File-=>Added oos_fin.patch, #19092

    Back to the top


    Powered by Savane 3.1-cleanup1