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

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Fri 30 Oct 2009 05:52:55 PM UTC
   
 
Category:  TCP 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

Jump to the original submission

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

Updated the wiki, too.

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group 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>
Group administrator
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>
Group 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>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group 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>
Group administrator
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>
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 jeffbabar (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 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.13-02a9.
    Corresponding source code