patchlwIP - A Lightweight TCP/IP stack - Patches: patch #3796, api_lib.c: patch for...

 
 

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

patch #3796: api_lib.c: patch for netconn_write(), fixes a possible race condition

Submitter:  None
Submitted:  Mon 07 Mar 2005 09:20:14 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  In Progress Privacy:  Public
Assigned to:  fbernon Originator Email:  -email is unavailable-
Open/Closed:  Closed Planned Release:  None

Jump to the original submission

Wed 11 Apr 2007 11:54:05 PM UTC, comment #14: 

I think the best thing to do to fix this kind of problem is to move most of the write process in tcpip_thread (in api_msg). I have submit a patch for lwip_sendto like this for bug #19162 (I have to test it when I will come to the office).

It will give a something more safe-thread (by example, if two application threads want to write datas on the same socket).

To my point of view, SYS_ARCH_PROTECT should only have to be used in pbuf, & memp functions...





Frédéric Bernon <fbernon>
Group Member
Wed 11 Apr 2007 11:40:24 PM UTC, comment #13: 

Would SYS_ARCH_PROTECT be a straightforward (albeit ugly) way to avoid the race here? tcp_sndbuf() is straightforward read of pcb->snd_buf, which is only assigned to in three places.

Alternatively there could be (another!) message to the tcpip thread to retrieve the send buffer size.

I think it wouldn't be too hard to fix this completely, even if it's not very clean.

Jonathan Larmour <jifl>
Group Member
Mon 26 Mar 2007 06:51:56 PM UTC, comment #12: 

Closed, but I think we will add a new "redesign api_lib\api_msg" in few times...

Frédéric Bernon <fbernon>
Group Member
Mon 26 Mar 2007 04:40:26 PM UTC, comment #11: 

I think the patch solves the problem and this bug can be closed. Tha problem that "size" overflows is fixed, and the rest of the code seems OK to me (as long as tcp_sndbuf() yields to an atomic instruction).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 26 Mar 2007 03:39:20 PM UTC, comment #10: 

Ok, Dmitry's patch is integrate. Not a final solution, but a good workaround for the moment...

Frédéric Bernon <fbernon>
Group Member
Mon 26 Mar 2007 01:11:34 PM UTC, comment #9: 

Ok, so, in a first time, I just integrate it in CVS like that...
 


Frédéric Bernon <fbernon>
Group Member
Mon 26 Mar 2007 11:00:43 AM UTC, comment #8: 

I'd rather wait for a more complete solution, as with this patch it might be "good enough" that no one gets round to a more complete solution, but without this we might be motivated to do it properly.

However, if this is causing people problems in the short term and adding this patch will help them make progress then it is hard to say no.

If you think it necessary then go ahead.  If you think we can manage without it then leave it out for now.

Kieran Mansley <kieranm>
Group Member
Mon 26 Mar 2007 10:29:44 AM UTC, comment #7: 

Kieran,

Is it ok to integrate Dmity's patch? I hope Simon will redesign this part later, but, the patch is a "good" temporary solution...

Frédéric Bernon <fbernon>
Group Member
Thu 08 Mar 2007 02:47:08 PM UTC, comment #6: 

The current code of netconn_write certainly has a bug. Of course, the real solution would be to move some part of netconn_write to TCP/IP thread, but it is not easy and requires a lot of changes.

I believe the proposed patch does solve the problem for most cases. Unfortunately, the explanation provided by the author of the patch is a bit difficult to follow.

The problem is in the following lines:

      if (size > tcp_sndbuf(conn->pcb.tcp)) {
  /* We cannot send more than one send buffer's worth of data at a
     time. */
  len = tcp_sndbuf(conn->pcb.tcp);
      } else {
  len = size;
      }

If the value of tcp_sndbuf(conn->pcb.tcp) is changed between verifying the condition and assigment to `len' then `len' can have value larger than size! So netconn_write will send some garbage. After that,  size -= len; will cause overflow of `size', so it will continue to send even more garbage. So 64Kb of garbage will be sent.

The solution proposed by the author of the patch is simple -- to evaluate tcp_sndbuf(conn->pcb.tcp) expression only once and to keep it in a local variable, which is always good thing to do, because it reduces size of the code and increases speed.

So I will recommend to apply this patch now, because more perfect solution may take some to be developed and the current patch will solve problems for most users.

Of course, the semaphore related stuff is no longer needed, so I remove it in a newly attached version.

(file #12149)

Dmitry Potapov <dpotapov>
Thu 08 Mar 2007 11:36:23 AM UTC, comment #5: 

Funny, I did not read this patch before, but last weekend I was worried about exactly that bit of code, accessing the pdb->snd_buf from the user thread.
As to your questions, Kieran:

- I don't think the patch is a solution, it only reduces the number of possible conflicts as sndbuf is not read as often as before

- the two threads seem to be tcpip_thread and an application thread using sockets or netconn-api.

- I think this IS a major problem, but it can only be solved by (at least slightly) redesigning the netconn/socket api

I'm interested in going into this, but it could take some time...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 19 Feb 2007 11:19:52 AM UTC, comment #4: 

I'm dubious about this for three reasons:
 - the patch doesn't increase synchronisation, it simply solves the problem of len overflowing if the snd_buf does change
 - it's not clear which two threads are causing the problem: in general lwIP is not thread safe, so if this is simply a restatement of that then it's not a useful fix
 - the fact that this is so old (sorry for the delay!) and hasn't been a widely reported problem, makes me think that it's not a major problem.

Any other developers care to comment?

Kieran Mansley <kieranm>
Group Member
Mon 19 Feb 2007 10:45:32 AM UTC, comment #3: 

This problem is still there, I think. I did have look at netconn_write() previously to see if it can be improved by redesign instead of patching, but I don't have the time for that. So maybe you can have a look at this patch while sweeping the others.

I do remember TCP echo returning garbage at file trailers, but I'm unsure if this is/are bug(s) here or elsewhere. Sorry can't help you with this, returning this to none.

Christiaan Simons <christiaans>
Group Member
Mon 19 Feb 2007 10:30:16 AM UTC, comment #2: 

Can anyone give an update on this.  Is it still a problem?  Did it get fixed?

Christiaan - as you said at the time you were investigating, I'm assigning this to you.  If you don't have time to look at it then  please say so.

Kieran Mansley <kieranm>
Group Member
Wed 15 Feb 2006 08:23:03 AM UTC, comment #1: 

I'm investigating netconn_write(), I'm also getting garbage when using the sequential API. I think netconn_write() also has a very poor performance because of the frequent thread ipc/synchronisation. Maybe some code must be moved to the tcpip thread (src/api/tcpip.c) instead of applying this patch.

Christiaan Simons <christiaans>
Group Member
Mon 07 Mar 2005 09:20:14 AM UTC, original submission:  

This patch is for lwIP-1.1.0.

Description:

In really seldom cases lwIP transmits up to 64kb garbage data in a TCP/IP stream when using the socket api.

After looking closer to api_lib.c, I have found the problem in the function netconn_write. The problem there is that a variable that is shared between the TCP thread and the user thread is accessed three times without synchronization of the two threads. The variable I am talking about is pcb->snd_buf, that is accessed by using the macro call tcp_sndbuf(conn->pcb.tcp). The function netconn_write tries to figure out how much space is still there in the send buffer, and transmitts the data piece by piece to ensure the send buffer does not overflow. But in the case of the race condition, the size is calculated wrong, and the variable 'len' can overflow. Since len is of type u16_t, lwIP sends now up to 64kb garbage data.

The patch fixes this particular problem, and a race condition problem with the semaphore in the same function.

Anonymous

 

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

Attached Files
file #12149:  netconn_write-2.patch added by dpotapov (1KiB - text/x-patch)
file #8541:  netconn_write.patch added by None (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by dpotapov (Updated the item)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by christiaans (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2007-03-26 fbernon Open/ClosedOpen Closed
    2007-03-26 fbernon StatusNone In Progress
        Assigned toNone fbernon
    2007-03-08 dpotapov Attached File- Added netconn_write-2.patch, #12149
    2007-02-19 christiaans Assigned tochristiaans None
    2007-02-19 kieranm Assigned toNone christiaans
    2006-05-02 christiaans Assigned tochristiaans None
    2006-02-15 christiaans Assigned toNone christiaans
    2005-03-07 None Attached File- Added netconn_write.patch, #4302
        Carbon-Copy- Added dennis_k --AT-- freenet --PUNKT-- de

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code