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

[Konrad, Guido]

David,

yes, the code below solves the problem. So is my only problem that the
access to
the CVS database is not allowed by our sysadmin. I assumed having the latest
code
but ...

Again thanks a lot to you all for your excellent help. 

Guido

-----Ursprüngliche Nachricht-----
Von: David Haas [mailto:address@hidden
Gesendet: Mittwoch, 28. Mai 2003 19:23
An: Mailing list for lwIP users
Betreff: Re: [lwip-users] Found bug in pbuf.c


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






_______________________________________________
lwip-users mailing list
address@hidden
http://mail.nongnu.org/mailman/listinfo/lwip-users