buglwIP - A Lightweight TCP/IP stack - Bugs: bug #56437, Stack/heap corruption when sending...

 
 

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

bug #56437: Stack/heap corruption when sending UDP through socket interface

Submitter:  Gideon Z <gideonz>
Submitted:  Tue 04 Jun 2019 06:35:26 PM UTC
   
 
Category:  UDP Severity:  3 - Normal
Item Group:  Crash Error Status:  Invalid
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.1

Tue 04 Jun 2019 09:14:47 PM UTC, comment #2: 

Thank you for your reply. I apologize. You are absolutely right about me using a modified stack. It has been 5 years since I made this [horrible] change. I verified most code pieces with the repository, but I missed this part; probably because I was too happy that I finally found the root cause of my crash.

// The comments are C90 / C99 style.

Gideon Z <gideonz>
Tue 04 Jun 2019 07:15:00 PM UTC, comment #1: 

I think there's only one problem: you're using a modified stack. I can't find that comment nor the code in our released sources.

Nit: that comment even uses C++ style. We're only using C-style comments ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 04 Jun 2019 06:35:26 PM UTC, original submission:  

When using lwIP through its socket interface, stack or heap corruption occurs when the parameter LWIP_NETIF_TX_SINGLE_PBUF is set to 0, and the user application tries to send a UDP packet.

The root cause is, that when LWIP_NETIF_TX_SINGLE_PBUF is 0, the stack decides to use a pbuf in reference mode, by calling _netbuf_ref_, in the function _lwip_sendto_. The payload pointer points to user data, possibly on the stack of the caller.

Later, within the tcpip_thread, the header is appended in front of this user data, because of the following code snippet in :

```  } else if (type == PBUF_REF || type == PBUF_ROM) {
  // just ALWAYS do this, since references are only used to read!
      p->payload = (u8_t *)p->payload - header_size_increment;
```

Nice comment.. Yes, the data is only read, but the area near it is written.. ;-)

I think there are actually TWO problems:

1) A pbuf reference should never point to the stack of a caller, because the actual sending of a packet happens in another thread. It would 'work' in this particular case, because the caller is stalled using the semaphore until the packet is sent. But if the driver queues the packets, OR when someone 'improves' the lwIP code by adding a queue mechanism, this will definitely break.

2) A PBUF_REF should never be chosen if a header needs to be pre-pended. It would be fine if the core would actually chain the PBUFs. But in case of a PBUF_REF it explicitly chooses not to.

This problem seems to be addressed in 2.1.2, but I checked: the UDP send still uses a PBUF_REF for the UDP send function. I believe, from analyzing the code, that it allocates a new pbuf and chains it, which would be OK. (Despite the fact that the code to do this should not be in udp.c; it doesn't belong there.)

Gideon Z <gideonz>

 

(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 gideonz (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-06-04 goldsimon StatusNone Invalid
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code