buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26405, Prematurely released semaphore...

 
 

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

bug #26405: Prematurely released semaphore causes lwip_select() to crash

Submitter:  Konstantin <konstua>
Submitted:  Thu 30 Apr 2009 07:19:38 AM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

Tue 05 May 2009 05:21:34 PM UTC, comment #5: 

Checked in. Thanks for reporting!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 05 May 2009 07:59:43 AM UTC, comment #4: 

Aaahh, timeout! That's the bit that was missing, I must have overread it...

Thank you so much for clarifying this, I'll check it in as soon as I find the time.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 05 May 2009 07:21:00 AM UTC, comment #3: 

Simon,
I don't think my port is a problem. It does indeed understand the initial state, here is the sys_sem_new() I am using (modelled after a sample provided with FreeRTOS):

sys_sem_t sys_sem_new(u8_t count)
    {
    xSemaphoreHandle xSemaphore;

    portENTER_CRITICAL();
    vSemaphoreCreateBinary( xSemaphore );
    if (count == 0)
        {
        xSemaphoreTake( xSemaphore, 1 );
        }
    portEXIT_CRITICAL();

    return xSemaphore;
    }


However, I think the problem may be that sys_sem_wait_timeout() may time out (hence the name). (In my case, the timeout was quite short.)
I am positive that in my case, a condition occured when by the time tcpip_thread signals selectsem, the application thread was already waiting in sys_sem_wait(selectsem), and as soon as the semaphore was released, the OS woke up the application thread which destroyed select_cb.sem.

Konstantin <konstua>
Mon 04 May 2009 08:02:20 PM UTC, comment #2: 

Reading the code, I don't get how it could go wrong:

tcpip_thread calls:
    sys_sem_signal(selectsem);
    sys_sem_signal(scb->sem);
while the application thread calls:
    i = sys_sem_wait_timeout(select_cb.sem, msectimeout);
    sys_sem_wait(selectsem);
    sys_sem_signal(selectsem);
    sys_sem_free(select_cb.sem);

The only problem I see here is that the port used by Konstantin could maybe ignore the int parameter passed to sys_sem_new and always initialize the semaphore with 1, in which case it would wait only for selectsem... but that would make problems elswhere, too.

Anyway, since it's good practice (or at least that's what I've learnt:) to use resources in the same order in every place, I don't have that strong an opinion against changing the order of the sys_sem_signal calls... Nevertheless, I'd like to understand the bug before making the change.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 30 Apr 2009 08:03:09 AM UTC, comment #1: 

Looks like a bug to me, and a good fix.  Well spotted.

Kieran Mansley <kieranm>
Group Member
Thu 30 Apr 2009 07:19:38 AM UTC, original submission:  

While trying to get lwIP to work on a FreeRTOS port for Blackfin BF53x, I have observed elusive memory corruption. After a lot of debugging I have traced the cause to the following code:

(sockets.c :: event_callback())

if (scb) {
      scb->sem_signalled = 1;
      sys_sem_signal(selectsem);
      sys_sem_signal(scb->sem);
    } else {
      sys_sem_signal(selectsem);
      break;
    }


It seems that, at least on my system, a situation occured where lwip_select() was waiting for selectsem in another thread, and as soon as it was released, lwip_select() could be exited, leaving scb (pointing at a local variable inside lwip_select()) an invalid reference.

Suggestion: swapping the two subsequent calls to sys_sem_signal() seems to resolve the problem:


if (scb) {
      scb->sem_signalled = 1;
      sys_sem_signal(scb->sem); /* -- before selectsem */
      sys_sem_signal(selectsem); /* -- may invalidate scb */
    } else {
      sys_sem_signal(selectsem);
      break;
    }


Konstantin <konstua>

 

(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 goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by konstua (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-05-05 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2009-05-05 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2009-04-30 kieranm Planned Release 1.3.1

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code