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

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Wed 28 Mar 2007 09:10:34 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  fbernon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
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>
Group Member
Wed 28 Mar 2007 10:13:28 AM UTC, comment #1: 

Patch file I propose to commit...

(file #12319)

Frédéric Bernon <fbernon>
Group Member
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>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

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
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by fbernon (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
    2007-03-28 fbernon StatusNone Fixed
        Open/ClosedOpen Closed
    2007-03-28 fbernon Attached File- Added netbuf_ref.patch, #12320
    2007-03-28 fbernon Attached File- Added netbuf_ref.patch, #12319

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code