buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27871, Calling tcp_abort() in recv...

 
 

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

bug #27871: Calling tcp_abort() in recv callback can lead to accessing unallocated memory

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Fri 30 Oct 2009 05:52:55 PM UTC  
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.1

( Jump to the original submission)

Fri 29 Jan 2010 02:18:34 PM UTC, comment #11: 

Updated the wiki, too.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 29 Jan 2010 10:30:09 AM UTC, comment #10: 

Maybe something on the wiki?  But doc/rawapi.txt is the main place, thanks.

Kieran Mansley <kieranm>
Project Member
Fri 29 Jan 2010 10:16:21 AM UTC, comment #9: 

I've already updated doc/rawapi.txt. Is there any other place of API documentation I forgot?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 29 Jan 2010 09:30:29 AM UTC, comment #8: 

Could you make sure the API documentation is updated to include this change?

Kieran Mansley <kieranm>
Project Member
Thu 28 Jan 2010 12:41:54 PM UTC, comment #7: 

Sounds good to me.  I like that it provides explicit feedback to the stack.

Jeff Barber <jeffbabar>
Wed 27 Jan 2010 05:24:08 PM UTC, comment #6: 

Fixed by checking the return value of TCP_EVENT_xxx and not accessing the pcb pointer after that if err == ERR_ABRT.

This effectively means that ERR_ABRT must only be used when the application has called tcp_abort() from withing the callback and when calling tcp_abort() from such a callback, ERR_ABRT must be returned (updated the docs and comments, too).

Is this OK for you all?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sun 22 Nov 2009 03:53:16 PM UTC, comment #5: 

I think a better change might be to disallow applications to call tcp_abort and instead process the return value of sent-, recv- and poll-callback functions to let the stack call tcp_abort when they return ERR_ABRT.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 20 Nov 2009 05:54:44 PM UTC, comment #4: 

> I think this is best solved with documentation rather than
> changing the code.


I don't know about that: tcp_abort is explicitly mentioned in rawapi.txt, so it might be used in some applications for a while now.

I don't think a 'small' change in that doc file will draw much attention and thus this might be a bug in many applications.

After all, from where should a raw-api application call tcp_abort() if not from one of the pcb callbacks? The only place would be a self-created timeout...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 02 Nov 2009 09:09:00 AM UTC, comment #3: 

I don't mind applications using the raw API being able to call tcp_abort() - the raw API is by design low level, so being able to do things that applications couldn't normally do is OK.  However, documenting cases like this where it is not possible to use tcp_abort() is probably wise.  i.e. I think this is best solved with documentation rather than changing the code.

Kieran Mansley <kieranm>
Project Member
Sat 31 Oct 2009 12:32:25 AM UTC, comment #2: 

That's true. But with the current layout of the headers, it's kind of hard to separate between functions for applications and internally used functions. I.e. it's not always clear what counts as API and what doesn't.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 30 Oct 2009 06:20:16 PM UTC, comment #1: 

Wouldn't it be better to simply deprecate use of tcp_abort from applications?

If one considers the usual unix behavior as an analog: an application can't abort a connection.  It simply closes the socket.  Once closed, the OS cleans up, sending the FIN and so forth, and can choose to reset the connection if, for example, it continues to get data destined for a socket that is no longer open.

For lwip, if an application wishes to close a session, it should call tcp_close and clear its send and recv callbacks.  It will not then get any further callbacks and so, as far as it's concerned, the session is gone.  lwip is then responsible for cleanup: FIN-sending and/or possibly RST, as appropriate.

Jeff Barber <jeffbabar>
Fri 30 Oct 2009 05:52:55 PM UTC, original submission:  

This isn't new but has been in there for a while:

When working on the raw api httpd, I found tcp_abort is called within the recv-callback (e.g. in an error state or when there's a second request to one pcb which thinks it's already closed). What I want to achieve is to force-close the pcb in this case resulting in an ACK. For this, I call tcp_abort and return ERR_ABRT.

However, the code calling TCP_EVENT_RECV() not prepared for this: since tcp_abort() deallocates the pcb, and the code calling TCP_EVENT_RECV() accesses the pcb after that call, unallocated memory is accessed.

In the default-recv case, this doesn't lead to a crash, as the memory is not yet used for another pcb...

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
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by jeffbabar (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-01-29 goldsimon Open/ClosedOpen => Closed
    2010-01-27 goldsimon StatusNone => Fixed
        Assigned toNone => goldsimon

    Back to the top


    Powered by Savane 3.5