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

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Thu 15 Oct 2009 02:23:01 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project Administrator
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>
Project AdministratorIn charge of this item.
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>
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 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 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 23 Jan 2010 03:06:35 PM UTCgoldsimonOpen/ClosedOpen=>Closed
    Sun 17 Jan 2010 04:21:46 PM UTCgoldsimonStatusNone=>Fixed
    Thu 15 Oct 2009 03:25:57 PM UTCkieranmPlanned Release1.3.2=>1.4.0
    Thu 15 Oct 2009 03:22:54 PM UTCkieranmStatusFixed=>None
      Open/ClosedClosed=>Open
    Thu 15 Oct 2009 02:34:09 PM UTCgoldsimonStatusNone=>Fixed
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1