buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19432, netbuf_ref doesn't check...

 
 

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

bug #19432: netbuf_ref doesn't check pbuf_alloc result

Submitted by:  Frédéric Bernon <fbernon>
Submitted on:  Wed 28 Mar 2007 09:10:34 AM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Frédéric Bernon <fbernon>
Open/Closed: ClosedPlanned Release: None
lwIP version: None

(Jump to the original submission Jump to the original submission)

Wed 28 Mar 2007 01:59:41 PM UTC, comment #7:

You are quite right, sorry.

Yes, go ahead as you describe.

Kieran Mansley <kieranm>
Project Administrator
Wed 28 Mar 2007 01:38:45 PM UTC, comment #6:

Ok to set "buf->ptr = NULL;" only if (buf->p == NULL), it's better.

About length fields, ???, what length field? I don't understand. There is no such field in netbuf. Or you talk about this line?

buf->p->len = buf->p->tot_len = size;

But if it's right, if buf->p==NULL, then we don't have len and tot_len.

Last, in struct netbuf, there is a "err_t err;" field which is never used. I propose to remove.

Ok to commit without the "err_t err;", and with "buf->ptr = NULL;" only if (buf->p == NULL)?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 28 Mar 2007 01:11:24 PM UTC, comment #5:

Ahh, OK. You're setting buf->ptr = NULL in case the alloc fails, to make sure it is a sensible value. In that case it should read:
if (buf->p != NULL) {
pbuf_free(buf->p);
}
buf->p = pbuf_alloc(PBUF_TRANSPORT, 0, PBUF_REF);
+ if (buf->p == NULL) {
+ buf->ptr = NULL;
+ return ERR_MEM;
+ }

As we only need to set this if there was an error. Do we also need to set the length fields in buf (to zero) if there was a problem allocating buf->p?

Kieran Mansley <kieranm>
Project Administrator
Wed 28 Mar 2007 01:06:01 PM UTC, comment #4:

>Should the new line:
>buf->ptr = NULL;
>be
>buf->p = NULL;


I don't think, because next line (the pbuf_alloc) will initialize (or set to NULL) the buf->p. Because in most of netbuf_ functions, like netbuf_alloc, netbuf_free, or even at the end of netbuf_ref, a "just initialized" netbuf have to get buf->p = buf->ptr. So, I think the patch is correct to this point of view.

The only caller is lwip_send, but the crash is directly inside netbuf_ref, because event if p==NULL efter pbuf_alloc, we call buf->p->payload.

I actually work on lwip_send (for the other bug report), but call should be something like :

/* make the buffer point to the data that should be sent */
if ((err = netbuf_ref(buf, data, size))==ERR_OK) {
/* send the data */
err = netconn_send(sock->conn, buf);
}

/* deallocated the buffer */
netbuf_delete(buf);

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 28 Mar 2007 12:30:55 PM UTC, comment #3:

Should the new line:
buf->ptr = NULL;
be
buf->p = NULL;

Also, could you update the caller(s) of netbuf_ref() to look at the return code to do something sensible if there's an error?

Kieran Mansley <kieranm>
Project Administrator
Wed 28 Mar 2007 10:18:32 AM UTC, comment #2:

Sorry, forget api.h

New patch file...

(file #12320)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 28 Mar 2007 10:13:28 AM UTC, comment #1:

Patch file I propose to commit...

(file #12319)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 28 Mar 2007 09:10:34 AM UTC, original submission:

In netbuf_ref, pbuf_alloc result is not tested. A check and a return value can be add...

err_t
netbuf_ref(struct netbuf buf, const void dataptr, u16_t size)
{ if (buf->p != NULL) {
pbuf_free(buf->p);
}
buf->ptr = buf->p = NULL;
buf->p = pbuf_alloc(PBUF_TRANSPORT, 0, PBUF_REF);
if (buf->p == NULL) {
return ERR_MEM;
}
buf->p->payload = (void*)dataptr;
buf->p->len = buf->p->tot_len = size;
buf->ptr = buf->p;
return ERR_OK;
}

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.

 

Attached Files
file #12320:  netbuf_ref.patch added by fbernon (2KiB - text/x-patch)
file #12319:  netbuf_ref.patch added by fbernon (782B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by fbernon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 28 Mar 2007 05:28:10 PM UTCfbernonStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Wed 28 Mar 2007 10:18:32 AM UTCfbernonAttached File-=>Added netbuf_ref.patch, #12320
    Wed 28 Mar 2007 10:13:28 AM UTCfbernonAttached File-=>Added netbuf_ref.patch, #12319

    Back to the top


    Powered by Savane 3.1-cleanup1