buglwIP - A Lightweight TCP/IP stack - Bugs: bug #21699, segment leak in ooseq processing...

 
 

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

bug #21699: segment leak in ooseq processing when last data packet was lost

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Sun 02 Dec 2007 04:42:12 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: None

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

Fri 14 Dec 2007 03:55:58 PM UTC, comment #7:

Yep, it's debug code to get a breakpoint... sorry for that. I'll fix it.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 14 Dec 2007 03:52:17 PM UTC, comment #6:

Hello, Simon,

today you have checked in tcp_in.c v1.89

First lines of your patch:

if (TCP_SEQ_LEQ(pcb->ooseq->tcphdr->seqno, seqno + inseg.len)) {
pcb->ooseq = pcb->ooseq;
}

Probably it is debug code. It does nothing.
Or did you meant something other?

Oleg Tyshev <olegreen>
Thu 13 Dec 2007 08:24:37 PM UTC, comment #5:

Fixed: just checked in the last patch (from comment #2).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 05 Dec 2007 01:42:40 PM UTC, comment #4:

Sounds sensible to me. Thanks for taking the time to investigate.

Kieran Mansley <kieranm>
Project Administrator
Tue 04 Dec 2007 08:38:34 PM UTC, comment #3:

As a start, I'll add an assert in tcp_pcb_remove() that checks whether any segments are left on unsent, unacked or ooseq.

Also, I'll change tcp_process a little (don't set closed state in tcp_process, tcp_input would do that anyway when calling tcp_pcb_remove), that way, the segment leak is fixed already.

But I'll still check in a 3rd fix for this (the one suggested in comment #2), but I want to do some more tests on this before checking in.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 03 Dec 2007 09:19:10 PM UTC, comment #2:

The 'proper' way to fix it (maybe a little bit too specialized for the scenario below) would be to change the code

inseg.len = (u16_t)(pcb->ooseq->tcphdr->seqno - seqno);
pbuf_realloc(inseg.p, inseg.len);

in tcp_receive into
if (pcb->ooseq->len > 0) {
/* We have to trim the second edge of the incoming
segment. */
inseg.len = (u16_t)(pcb->ooseq->tcphdr->seqno - seqno);
pbuf_realloc(inseg.p, inseg.len);
} else {
/* does the ooseq segment contain only flags that are in inseg also? */
if ((TCPH_FLAGS(inseg.tcphdr) & (TCP_FIN|TCP_SYN)) ==
(TCPH_FLAGS(pcb->ooseq->tcphdr) & (TCP_FIN|TCP_SYN))) {
struct tcp_seg *old_ooseq = pcb->ooseq;
pcb->ooseq = pcb->ooseq->next;
memp_free(MEMP_TCP_SEG, old_ooseq);
}
}

which removes the last segment from ooseq if it contains no data and the flags that count as segno (SYN & FIN) are the same for both segments.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 03 Dec 2007 09:06:20 PM UTC, comment #1:

It seems that not only the ooseq processing has a problem (in fact, that is not really a problem).

In the scenario below, tcp_pcb_remove is called before freeing the pcb ('if (recv_flags & TF_CLOSED)' in tcp_input), which should free all the lists. However, tcp_pcb_purge (called from tcp_pcb_remove) does nothing in the CLOSED state (as well as in the TIME_WAIT state). Changing this check solves this bug.

Therefore, I would rather solve the bug that way instead of checking the ooseq for flags (like FIN) that are received twice (the code would be more complex there). But maybe that is not the cleanest thing to do?

Any lwIP TCP experts have to say something about this? Kieran?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sun 02 Dec 2007 04:42:12 PM UTC, original submission:

See this mail from Thomas Catalino for a detailed description: http://lists.nongnu.org/archive/html/lwip-users/2007-12/msg00003.html

scenario: the last data packet (not containing FIN) is lost and the FIN packet (not containing data) is put on ooseq. Then the data packet is retransmitted with FIN. ooseq handling in tcp_receive processes trimming of segments but does not correctly handle this case (since data length is not affected but sequence numbers are).

It is detected that there is something do trim (in tcp_receive, line 1016), but no real action is taken).

This results in the first received FIN segment leaking.

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

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 13 Dec 2007 08:24:37 PM UTCgoldsimonStatusConfirmed=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1