Re: [lwip-users] Bug of ARP?

[Leon Woestenberg]

Hello Jason,

thanks for your bug report.

Did you experience premature free()ing of pbufs?

I have tried following your analysis. Maybe you do have a point that some
packets could be free prematurely, but I think the problem might be
somewhere
else, and I think your patch fixes the wrong stuff.


Wei Bo-Er (Jason) wrote:

> Hi all,
> I'm using lwIP 1.0.0 and found a problem look like a lwIP's bug. I
> don't know if there is
> anyone had mentioned it before(There seems to be many bugs fixing
> recently). I just explain
> what I found here.

Out of interest, you found this in what way? Actual debugging or code
analysis?

Also, did you *see* this happen with TCP only, or UDP and ICMP as well?

> In gereral, LwIP will queue packets as there is no matching entry in
> the ARP table. It uses

Yes, more exactly, only if ETHARP_QUEUEING is 1, this downstream
queueing occurs
(if resources are available).

> p = pbuf_take(q) to copy sent packet 'q' to internal pbuf 'p' and make
> q=p.The 'p' will be

The documentation of pbuf_take() says:

* Go through a pbuf chain and replace any PBUF_REF buffers
* with PBUF_POOL (or PBUF_RAM) pbufs, each taking a copy of
* the referenced data.
* ...
* @note Any replaced pbufs will be freed through pbuf_free().
* This may deallocate them if they become no longer referenced.

So, it only makes copies for PBUF_REF type pbufs. This makes sure the
caller can safely re-use
the memory references by PBUF_REF pbufs after submitting them to lwIP
for transmission.

For non-PBUF_REF types, nothing happens.

> queued in arp_table[i].p and be freed after timeout or it can be sent
> out. This will cause some
> problems in TCP. Since TCP needs to keep sent packets in unacked queue
> until ack is
> arriving, those packets may be freed in ARP before ack is received by
> TCP. Except TCP,

Maybe TCP should reference the pbufs it decides to queue and free them
when no longer
queueing them?

> ICMP and UDP will also free sent packets after return of sent-out
> function. It may also
> causes packets in ARP queue be freed before timeout or they can be
> sent out.

To prevent that, etharp increments the pbuf reference count when
queueing it.

> I made some modification in pbuf_take which is shown as follow to
> resolve it.
> In pbuf.c
> 828 struct pbuf *
> 829 pbuf_take(struct pbuf *p)
> 830 {
> 831 - struct pbuf *q , *prev, *head;
> + struct pbuf *q , *prev, *head, *ori_head;
> .....
> 836 - head = p;
> + head = ori_head = p;
> .....
> 841 - pbuf_free(p);
> .....
> 911 + p=ori_head;

This patch does nothing except remove pbuf_free() !

Please note that p has local scope and it not used afterwards.

> 912 return head;
> 913 }
> This modification keeps 'p' as original one and doesn't free it. All
> sent packets will be freed by up layer
> instead of ARP.

I think the real problem is that the upper TCP layer should increment
the reference count of a pbuf during
queueing and decrement it once it decides to drop it (by receiving an
ACK for it).

UDP and ICMP do not queue and have no extra references to a pbuf.

Maybe I am missing the point. Please let me know on the list.

Regards,

Leon.