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.
|
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.
|
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?
|
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?
|
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.
|
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?
|
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.
|
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.
|
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
|