Re: [lwip-users] Found bug in pbuf.c

[David Haas]

We spent a lot of time debugging pbuf memory leak issues and I actually
thing we do have it right now. While there certainly could be some bugs,
I have pounded on basic tcp connections and have run continuous data
across the connections for several days without any memory leaks. I am
really very confident that there are no basic memory leaks in the pbuf code.

The general idea is that pbuf->ref should be incremented for each
reference made to a pbuf. That reference could be a pointer that tcp or
an application holds or it could be from another pbuf in a chain. The
operation you describe where a pbuf is chained should result in the
second pbuf having a ref count of 2 and the first one having a ref count
of 1. If the application  who calls pbuf_chain() does not need to keep a
pointer to the second pbuf, then it may now safely free it (the second
pbuf). That is what happens in the tcp_enqueue() code you are describing.

My version of tcp_out.c looks like this around line 211:

     ++queuelen;

     /* Chain the headers and data pbufs together. */
     pbuf_chain(seg->p, p);
     pbuf_free(p);

If your version looks different, then I don't think you are using the
latest lwip.

Are you saying that there is a memory leak only from code inspection or
are you really finding a problem in operation?

David.


Konrad, Guido wrote:

> Hello Leon,
>
> I am not sure but please think about the following:
>
> Two pbufs are allocated and chained together with pbuf_ref(t). After that,
> the ref count of the first pbuf is 1, the count of the second is 2. For
> three pbufs, you would see ref count 1, 2, 2.
> Example: tcp_out.c in line 211, if you use netconn_write(... _NOCOPY). 
>
> If you now try to free the chained pbuf, the code below decrements the ref
> count of every chain member by one, means that only the first is freed. Do
> you agree?
> Example: tcp_seg_free(next); in tcp_in.c after ACK frees only the first
> pbuf.
>
> Please comment,
>
> Regards,
> -- Guido Konrad
>
> -----Ursprüngliche Nachricht-----
> Von: Leon Woestenberg [mailto:address@hidden
> Gesendet: Mittwoch, 28. Mai 2003 01:13
> An: Mailing list for lwIP users
> Betreff: Re: [lwip-users] Found bug in pbuf.c
>
>
> Hello Guido,
>
>  
>
> > in my last mail I asked for help due to a problem with memory leaks. It
> > seems to me that this problem is related to the ref counter in pbufs if
> > pbufs are chained. Function pbuf_chain increments the ref counter of the
> > chained pbuf, (e.g. the second), but pbuf_free does not decrease the ref
> > counter. So only the first pbuf of a chain is freed.
> >
> >    
> I have to disagree with that, but please see if I am correct.
>
> Just after your added fix (see below), it reads "p = q;"
>
> There, p is assigned to point to the next pbuf in chain (stored in q).
>
> The while loop then... well... "loops" and p->ref-- is performed on
> the second pbuf, which will be freed also if its ref count reaches zero.
>
> So, I think your fix is wrong, and this function is OK.
>
> The loop effectively does this:
>
>  /* de-allocate all consecutive pbufs from the head of the chain that
>   * obtain a zero reference count after decrementing once*/
>
>  
>
> >    while (p != NULL) {
> >        /* all pbufs in a chain are referenced at least once */
> >        LWIP_ASSERT("pbuf_free: p->ref > 0", p->ref > 0);
> >        p->ref--;
> >        /* this pbuf is no longer referenced to? */
> >        if (p->ref == 0) {
> >            /* remember next pbuf in chain for next iteration */
> >            q = p->next;
> >            /* is this a pbuf from the pool? */
> >            if (p->flags == PBUF_FLAG_POOL) {
> >                p->len = p->tot_len = PBUF_POOL_BUFSIZE;
> >                p->payload = (void *)((u8_t *)p + sizeof(struct pbuf));
> >                PBUF_POOL_FREE(p);
> >                /* a RAM/ROM referencing pbuf */
> >            } else if (p->flags == PBUF_FLAG_ROM || p->flags ==
> > PBUF_FLAG_REF) {
> >                memp_freep(MEMP_PBUF, p);
> >                /* pbuf with data */
> >            } else {
> >                mem_free(p);
> >            }
> >            count++;
> >            /* proceed to next pbuf */
> > gko->       if( q != NULL ) q->ref--;
> >            p = q;
> >            /* p->ref > 0, this pbuf is still referenced to */
> >            /* (so the remaining pbufs in chain as well)    */
> >        } else {
> >            /* stop walking through chain */
> >            p = NULL;
> >        }
> >    }
> >
> >
> > Please comment.
> >
> >    
> I would suggest putting some extra debug outputs here if you can debug at
> all.
>
> I am very interested in knowing what is going on, as the stack should be
> very
> stable by now.
>
> With regards,
>
> Leon Woestenberg.
>
>
>
>
> _______________________________________________
> lwip-users mailing list
> address@hidden
> http://mail.nongnu.org/mailman/listinfo/lwip-users
>
>
> _______________________________________________
> lwip-users mailing list
> address@hidden
> http://mail.nongnu.org/mailman/listinfo/lwip-users
>
>
>