buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27709, conn->err race condition on...

 
 

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

bug #27709: conn->err race condition on netconn_recv() timeout

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Thu 15 Oct 2009 02:23:01 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.1

Sun 17 Jan 2010 04:21:46 PM UTC, comment #5: 

Fixed by changing netconn_recv() to return err_t.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Oct 2009 04:33:04 PM UTC, comment #4: 


> I don't think we've really fixed this - just made the race more unlikely.


I'm not sure there still is a race condition: The code now doesn't rely on conn->err to be something specific, the only place where conn->err is checked is to not overwrite another error (only write ERR_CLSD if it is ERR_OK before). The original problem is thus solved.

However, I do agree that changing the API to let netconn_recv() return err_t would be good. We should then also change the netconn functions to return an err_t inside struct api_msg and we could eliminate conn->err totally.

I think we should find another way to solve the problems we tried to fix by ERR_IS_FATAL(conn->err), e.g. test if a connection is opened before sending or receiving.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Oct 2009 03:25:57 PM UTC, comment #3: 

I think I can see why netconn_recv is different: all the other netconn functions return an err_t, whereas netconn_recv returns a buffer, so has to use conn->err to indicate error status.

I think we should modify netconn_recv to return an error like the others, and have it take a buffer pointer as an argument to the function.  This will involve an API change, but I think is justifiable given the nature of this bug.  However, because there will be an API change I'll mark this as 1.4.0

Kieran Mansley <kieranm>
Group Member
Thu 15 Oct 2009 03:22:54 PM UTC, comment #2: 

I don't think we've really fixed this - just made the race more unlikely.

The fix is to stop conn->err being modified by two different threads.  netconn_recv is the only operation in api_lib.c that seems to be modifying conn->err.  If all the other operations can manage without changing it, I think netconn_recv should be able to too.

Kieran Mansley <kieranm>
Group Member
Thu 15 Oct 2009 02:34:09 PM UTC, comment #1: 

Fixed by directly returning when sys_arch_mbox_fetch times out. Thanks for reporting, David.

Still, the question of why we handle conn->err that way remains...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Oct 2009 02:23:01 PM UTC, original submission:  

This was reported by David Shmelzer on lwip_users:

First mail:

>>>>>> snip >>>>>>

I upgraded to 1.3.1 and it fixed the zero timeout problem I was having. Thanks.
However, I'm still getting a peculiar issue where netconn_recv erroneously sets conn->err to ERR_CLSD after 3 or so packets in the following section:


    /* If we are closed, we indicate that we no longer wish to use the socket */
    if (p == NULL) {
      memp_free(MEMP_NETBUF, buf);
      /* Avoid to lose any previous error code */
      if (conn->err == ERR_OK) {
-->     conn->err = ERR_CLSD;
      }
      return NULL;
    }

I put a breakpoint there and determined that the preceding call to sys_arch_mbox_fetch() returns timeout so conn->err should be ERR_TIMEOUT and not ERR_OK.
So somewhere between sys_arch_mbox_fetch() and the above code conn->err is being set to ERR_OK.
If I send another packet, ignoring the closed error, it fixes itself.
I then receive the preceding packet and the one just sent.
I'm calling netconn_recv() in a loop in another thread with a zero timeout.
help?
<<<<<< snap <<<<<<

Second mail:

>>>>>> snip >>>>>>

I traced the problem to conn->err being assigned a value in separate
threads.
One in recv_tcp() and another in netconn_recv().

recv_tcp() is called from the tcpip_thread and netconn_recv() is called
from a separate thread.
Am I using netconns incorrectly?
If so, shouldn't conn->err be a thread safe variable since it is
accessed from tcpip_thread as well as the thread using the netconn api?
conn->recv_avail is thread safe.
<<<<<< snap <<<<<<

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

    Date Changed by Updated Field Previous Value => Replaced by
    2010-01-23 goldsimon Open/ClosedOpen Closed
    2010-01-17 goldsimon StatusNone Fixed
    2009-10-15 kieranm Planned Release1.3.2 1.4.0
    2009-10-15 kieranm StatusFixed None
        Open/ClosedClosed Open
    2009-10-15 goldsimon StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code