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

Submitter:  Oleg Tyshev <olegreen>
Submitted:  Thu 19 Nov 2009 04:55:13 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

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

 

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

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

    Date Changed by Updated Field Previous Value => Replaced by
    2009-11-29 goldsimon Open/ClosedOpen Closed
    2009-11-29 goldsimon Assigned toNone goldsimon
        StatusNone Fixed
    2009-11-23 olegreen Attached File- Added oos_fin.patch, #19118
    2009-11-23 olegreen Attached File- Added oos_fin.patch, #19117
    2009-11-20 olegreen Attached File- Added oos_fin.patch, #19095
    2009-11-20 olegreen Attached File- Added oos_fin.patch, #19092

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code