buglwIP - A Lightweight TCP/IP stack - Bugs: bug #56077, len member wrong in pool allocated...

 
 

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

bug #56077: len member wrong in pool allocated pbuf

Submitter:  Karl Heinz Buchegger <kbuchegg>
Submitted:  Fri 05 Apr 2019 11:52:38 AM UTC
   
 
Category:  pbufs Severity:  3 - Normal
Item Group:  None Status:  Invalid
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Wed 11 Dec 2019 08:57:09 PM UTC, comment #4: 

I suspect this is from reading the code and it's invalid. memp_std.h creates the PBUF_POOL with LWIP_PBUF_MEMPOOL, not LWIP_MEMPOOL. This macro adjust the pool item's size from PBUF_POOL_BUFSIZE to (PBUF_POOL_BUFSIZE + sizeof(struct pbuf)), so PBUF_POOL_BUFSIZE is really the size available for payload.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 30 Apr 2019 11:47:35 AM UTC, comment #3: 

I don't really get what you're trying to say.

Can you provide a piece of code to reproduce what you think the problem is? Ideally on the win32 or linux port.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 Apr 2019 02:30:06 PM UTC, comment #2: 

I am sorry, I made a mistake.
I was looking at version 2.0.3 and not at the head version.

I just checked the head version. Although the code is different, it suffers from the same problem: In case of PBUF_POOL the len field has a wrong value and is to large for the amount of memory available.

Karl Heinz Buchegger <kbuchegg>
Fri 05 Apr 2019 12:00:16 PM UTC, comment #1: 

Also I would like to suggest a clearification in the documentation on the meaning of the len (and total_len) member, since
/** length of this buffer */
is a little bit to vague and in case of PBUF_POOL is not identical to "length of payload"

Karl Heinz Buchegger <kbuchegg>
Fri 05 Apr 2019 11:52:38 AM UTC, original submission:  

Can someone please confirm if this is really a bug or am I missing something

According to my understanding of the meaning of the len (and total_len) member in struct pbuf, they both describe the actually usable length of the payload memory (or the total of it)
From studying the source code all over lwip, this is my impression of the meaning. Note that this is the only interpretation which makes sense from the point of view of the user of a pool allocated pbuf chain.

if this is true, then there is a bug in the case of a PBUF_POOL allocated buffer.

Instead of
    p->len = LWIP_MIN(length, PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(offset));
   
it has to read
    p->len = LWIP_MIN(length, PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(SIZEOF_STRUCT_PBUF + offset));


and further down, instead of
    q->len = LWIP_MIN((u16_t)rem_len, PBUF_POOL_BUFSIZE_ALIGNED);
   
it has to read
      q->len = LWIP_MIN((u16_t)rem_len, PBUF_POOL_BUFSIZE_ALIGNED - SIZEOF_STRUCT_PBUF);
     
(that is in both cases: subtract the size of the pbuf struct from the available payload length also, since the pbuf structure is allocated inside the memory obtained from the pool allocator, and thus this memory is not usable for storing payloads)

Also note, that there is a bug in the immediatly following assertions.
The first one reads
LWIP_ASSERT("check p->payload + p->len does not overflow pbuf",
                ((u8_t*)p->payload + p->len <=
                 (u8_t*)p + SIZEOF_STRUCT_PBUF + PBUF_POOL_BUFSIZE_ALIGNED));

which seems to take care of the SIZEOF_STRUCT_PBUF as beeing not part of the usable payload length, but does not include the offset.

while the second one (inside the loop allocating the remaining required length) reads
LWIP_ASSERT("check p->payload + p->len does not overflow pbuf",
                  ((u8_t*)p->payload + p->len <=
                   (u8_t*)p + SIZEOF_STRUCT_PBUF + PBUF_POOL_BUFSIZE_ALIGNED));

which is totaly flawed, since the thing the assertion is all about is pointed to by q and not by p (but interestingly supports the understanding that SIZEOF_STRUCT_PBUF is not included in the value of len)

Once the len member is corrected, total_len will be correct automatically and more important, pbuf_alloc will alloacte a chain of pool allocated pbuf's with the correct size for storing the requested amount of bytes as payload.


Karl Heinz Buchegger <kbuchegg>

 

(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 kbuchegg (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-12-11 goldsimon StatusNeed Info Invalid
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2019-04-30 goldsimon StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code