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

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Sun 02 Dec 2007 04:42:12 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

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

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2007-12-13 goldsimon StatusConfirmed Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code