buglwIP - A Lightweight TCP/IP stack - Bugs: bug #31177, tcp timers can corrupt...

 
 

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

bug #31177: tcp timers can corrupt tcp_active_pcbs in some cases

Submitter:  Dave Wilson <dawilson>
Submitted:  Thu 30 Sep 2010 08:49:10 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

Fri 25 Nov 2011 05:39:06 PM UTC, comment #9: 

Fixed with the proposed patch.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 25 Nov 2011 05:15:12 PM UTC, comment #8: 

Change the topic to reflect that this can affect tcp_fasttmr(), too.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 25 Nov 2011 03:38:38 PM UTC, comment #7: 

Sounds reasonable.  We don't expect this to happen often after all.

Kieran Mansley <kieranm>
Group Member
Fri 25 Nov 2011 05:25:58 AM UTC, comment #6: 

I've prepared a patch for this which solves this by:

- putting all callback-calls at the end of tcp_slow/fasttmr
- marking a pcb as timer-touched at the start of the while-loop in the timer functions (using an extra counter that is increased at the very start of the timer functions)
- restarting the while-loop (from the beginning of the pcb list) if the active-pcb-list has been changed during callbacks
- skipping pcbs that are already marked as touched.

This is only neede for the active pcb list. It leads to traversing the active-pcb list multiple times if it is changed by a callback - but it's the only possiblity I could think of to give application code the freedom to do so in any callback.

(file #24447)

Simon Goldschmidt <goldsimon>
Group administrator
Sat 03 Sep 2011 08:55:16 PM UTC, comment #5: 

Thinking about this, the only really safe way to fix this is to keep a "revision" of the list we are traversing, remember that revision in the loop (before calling an application callback) and comparing it later.

If it has changed, we need to re-traverse the whole list. To prevent handling pcbs multiple times, I'd add 2 u8_t to struct tcp_pcb that hold the value 'tcp_timer' when the pcb has last been handled by slow_tmr (or fast_timer, respectively).

However, if 1.4.1 is to be a bugfix release, I'd regard this as too much of a code change to make it into that release.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 11 Jan 2011 02:01:13 PM UTC, comment #4: 

I think this relates as much to CVS head as to 1.3.2, so updating summary.

If anyone has a complete patch I'd take it for 1.4.0, but marking as planned for 1.4.1 as I don't think this is a gating issue

Kieran Mansley <kieranm>
Group Member
Mon 22 Nov 2010 03:44:46 PM UTC, comment #3: 

Thanks, Simon - it makes a difference when you really know the guts of the stack! I had not considered this possibility at all. I'll leave this fix in my copy for now since it definitely works better than the previous version for us. I will, however, go back in and rework based on your feedback when I have a chance.

By the way, is there any recommendation regarding what calls are safe from within the application callback function? Changing this particular application so that it doesn't make various calls would likely be a major effort.

Dave Wilson <dawilson>
Sat 20 Nov 2010 07:17:39 PM UTC, comment #2: 

The problem I see here is that when allocating a new pcb from one of those callbacks, any of the pcbs on the list can disappear if the MEMP_TCP_PCB pool is empty and tcp_alloc() calls tcp_kill_prio(). The pcb aborted by tcp_kill_prio() might even be the one we want to check next ('pcb').

Therefore, the only safe solution I see is to loop and search for both 'pcb' and 'prev' after calling a client callback.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 01 Oct 2010 12:33:44 PM UTC, comment #1: 

In reviewing this change, I realised that I had not taken into account the case where the active PCB list is empty after the PCB is removed. As a result, the suggested fix should actually be:

      prev = tcp_active_pcbs;
      while(prev && (prev->next != pcb))
      {
          prev = prev->next;
      }

Dave Wilson <dawilson>
Thu 30 Sep 2010 08:49:10 PM UTC, original submission:  

If a client error callback function (registered using tcp_err) allocates a new PCB then function tcp_slowtmr() can corrupt tcp_active_pcbs in some cases. This was seen in an application which makes 2 connections to a telnet server. In cases where the server is down when the initial connection attempt is made and tcp_slowtmr() detects the "Max SYN retries" case and removes the PCBs from tcp_active_pcbs list, we got into a state where one of the 2 connections was never reestablished when the server came back online.

Investigation showed that the root cause of the problem was an assumption that local variable "prev" would not need updated after a PCB is removed (around line 755 or thereabouts of tcp.c). This is correct if the application error callback doesn't allocate any PCBs but, if it does, this leads to problems. In our case, the wrong PCB was removed on the next PCB loop iteration and our connection "died".

To fix the problem, the following code can be added immediately after the line "pcb = pcb2;" in tcp_slowtmr().

      /* Determine the previous PCB in the list.  We
         need to do this in case the client error handler
         did something that allocates a new PCB. If this
         happens and the PCB we just freed was at the head
         of the list, we end up with a bad value in prev and
         can corrupt the list. */
      prev = tcp_active_pcbs;
      while(prev->next != pcb){
        prev = prev->next;
      }

Dave Wilson <dawilson>

 

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

Attached Files
file #24447:  fix_31177.patch added by goldsimon (10KiB - 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 kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dawilson (Submitted the item)
  • -email is unavailable- added by dawilson (Dave Wilson)
  •  

    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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-11-25 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2011-11-25 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
        Summarytcp_slowtmr() can corrupt tcp_active_pcbs in some cases. tcp timers can corrupt tcp_active_pcbs in some cases
    2011-11-25 goldsimon Attached File- Added fix_31177.patch, #24447
    2011-01-11 kieranm Planned Release 1.4.1
        SummarylwIP 1.3.2 tcp_slowtmr() can corrupt tcp_active_pcbs in some cases. tcp_slowtmr() can corrupt tcp_active_pcbs in some cases.
    2010-09-30 dawilson Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code