buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27034, Invalid ASSERT in pbuf_alloc()

 
 

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

bug #27034: Invalid ASSERT in pbuf_alloc()

Submitter:  Iordan Neshev <iordan_neshev>
Submitted:  Wed 15 Jul 2009 12:45:31 PM UTC
   
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Mon 27 Jul 2009 03:18:11 PM UTC, comment #8: 

Fixed by using the assert suggested in the initial post to prevent breaking PPP.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 16 Jul 2009 07:50:20 AM UTC, comment #7: 

No, because they're also asking for space to be reserved for headers (the first argument to pbuf_alloc), so the resulting length won't be zero.  It's only when called as pbuf_alloc(PBUF_RAW, 0, ...) that it's a problem I think.

Kieran Mansley <kieranm>
Group Member
Thu 16 Jul 2009 07:09:29 AM UTC, comment #6: 


>As said on lwip-devel, I'd just change the assert for releasing
>1.3.1 and think about the PPP code after the release.


OK, we agreed that using zero-length pbufs from
PPP is bad, but at least it works and does not crash. While we can close our eyes for PPP until 1.3.1 release, how about the other uses in the upper layers of the stack? I mean:

api\netbuf.c:
buf->p = pbuf_alloc(PBUF_TRANSPORT, 0, PBUF_REF);

api\sockets.c
p = pbuf_alloc(PBUF_TRANSPORT, 0, PBUF_REF);

raw.c:
q = pbuf_alloc(PBUF_IP, 0, PBUF_RAM);

ip_fraq.c:
rambuf = pbuf_alloc(PBUF_LINK, 0, PBUF_REF); // line 656
newpbuf = pbuf_alloc(PBUF_RAW, 0, PBUF_REF); // line 721
header = pbuf_alloc(PBUF_LINK, 0, PBUF_RAM); // line 755

>You should know the length before calling pbuf_alloc() and pass >it instead of 0.


Obviously the authors of the mentioned lines intentionally created zero-length pbufs. Is this illegal use?

Iordan Neshev <iordan_neshev>
Thu 16 Jul 2009 05:50:49 AM UTC, comment #5: 

As said on lwip-devel, I'd just change the assert for releasing 1.3.1 and think about the PPP code after the release.

Kieran, as I'm currently in a bad position regarding CVS access, could you check it in?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 15 Jul 2009 04:27:32 PM UTC, comment #4: 

Hm, if it really only checks "PBUF_POOL_BUFSIZE must be bigger than MEM_ALIGNMENT", then we'd better move the check into init.c so that it isn't executed always.

However, I'd like to know what the purpose of

  pbuf_alloc(PBUF_RAW, 0, PBUF_POOL);

is? Since there is no API that supports increasing a pbuf's length without moving the payload to the front (which you can't in this case, because PBUF_RAW is used), you'll end up with a pbuf of zero length, which is bound to lead to problems in other parts of the stack. There are checks for pbuf->len not being zero somewhere... (although I don't remember where and why).

I'd like to have this sorted out before changing the assert.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 15 Jul 2009 03:15:00 PM UTC, comment #3: 

Thanks for the extra information.

Simon: as you added this assertion, are you happy with the suggested change?

Kieran Mansley <kieranm>
Group Member
Wed 15 Jul 2009 02:57:45 PM UTC, comment #2: 


>Just to ensure I understand this properly: the assertion
>incorrectly fires when you try to allocate a pbuf
>with zero length?


Yes,that's right.

> Is there any other requirement to trigger the incorrect
> assertion?


No. (In fact only when type == PBUF_POOL, but this is obvious)

For me it happens from the PPP code, for example in ppp.c:
#if PPPOS_SUPPORT
  headMB = pbuf_alloc(PBUF_RAW, 0, PBUF_POOL);
  ....

but I see several other places where pbuf_alloc() is called with len=0, for ex. ip_fraq.c, raw.c...


Last time I updated lwip in my project was 2 months
ago. Now I update again before the next release and I see this ASSERT added.

I tested it with the proposed change and it's OK.
The device works as expected, nothing bad happens till now.


Iordan Neshev <iordan_neshev>
Wed 15 Jul 2009 02:42:54 PM UTC, comment #1: 

Just to ensure I understand this properly: the assertion incorrectly fires when you try to allocate a pbuf with zero length?  Is there any other requirement to trigger the incorrect assertion?

This is something that the stack does reasonably frequently, so I'm trying to understand why it hasn't been seen by others.

Kieran Mansley <kieranm>
Group Member
Wed 15 Jul 2009 12:45:31 PM UTC, original submission:  

In pbuf.c:
struct pbuf *
pbuf_alloc(pbuf_layer layer, u16_t length, pbuf_type type)
{
...
  switch (type) {
  case PBUF_POOL:
...
 p->len = LWIP_MIN(length, PBUF_POOL_BUFSIZE_ALIGNED -
                     LWIP_MEM_ALIGN_SIZE(offset));
...
LWIP_ASSERT("PBUF_POOL_BUFSIZE must be bigger than
              MEM_ALIGNMENT", p->len != 0);
}

The logical assert is really
(PBUF_POOL_BUFSIZE_ALIGNED > LWIP_MEM_ALIGN_SIZE(offset)).

I guess it was replaced with (p->len != 0) with the
intention to eliminate the overhead of evaluating the
LWIP_MEM_ALIGN_SIZE macro again. But when the requested
lenght is 0 this assert is wrong.

If I am right why this was done (to eliminate the overhead),
then the expression (PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(offset)) could be explicitly
evaluated only once by storing the result in a local variable. This adds a few lines of source which I dislike. Alternatively
it may become:

LWIP_ASSERT("PBUF_POOL_BUFSIZE must be bigger than MEM_ALIGNMENT", (PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(offset)) > 0);
relying on the optimizer to evaluate it once.
I'm sure it will do it especially if
this ASSERT is moved right before
p->len = LWIP_MIN(length, ...

On the other hand it's (almost)worthless to optimize
tests in assertions since in the field they are
usually disabled.

I propose to replace
p->len != 0
with
(PBUF_POOL_BUFSIZE_ALIGNED - LWIP_MEM_ALIGN_SIZE(offset)) > 0

Greetings,
Iordan

Iordan Neshev <iordan_neshev>

 

(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 iordan_neshev (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
    2009-07-27 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2009-07-15 kieranm Planned Release 1.3.1

    Back to the top

    Powered by Savane 3.13-bb6a.
    Corresponding source code