[lwip-users] Fw: lwip-issues

[Leon Woestenberg]

Hello all,

the following issues have been reported and fixed in the current CVS.

I found it very time consuming to enter them into the bug database, as
I have fixed and closed them already. Sorry, short on time.

Thanks to Anders Carlman for reporting them!

Regards,

Leon Woestenberg.


----- Original Message -----
From: "Anders Carlman" <address@hidden>
To: "Leon Woestenberg" <address@hidden>
Sent: Sunday, March 16, 2003 3:57 PM

> > In brief, the contents of the previous messages was that I have
discovered
> bugs that cause memory leaks in the etharp and dhcp code.
>
> Can you say which release of lwIP you are referring to? If this is the
> latest CVS, I'm very interested to hear about the memory leak bugs!

My "working version" is quite recent. And just to be sure, I took a quick
look at the cencerned source files through the web-based CVS, and the bugs
are still there...

The first bug appears in the etharp_query()-function in etharp.c if you are
using ARP_QUEUEING. The local variable "i" that indexes the arptable entry
is reused in two for-loops (line 782 and 795), which is probably ok i you're
not using ARP_QUEUEING. But if you do, the "i"-variable will always be set
to 6 when reaching the last part of the etharp_query(), and this will cause
pbufs to be "lost" (and other problems too, most likely)... look at line 811
and 817 and you will see what I mean...

My solution to this is to simply declare an additional "i"-variable inside
the scope of the if-clause at line 778. This will cause the for-loops to use
the "inner" i-variable, while the "original" i-variable will remain intact
until the end of the function... Like this:
-----------------
...
...
/* could allocate pbuf? */
  if (p != NULL) {
    u8_t i;
    DEBUGF(ETHARP_DEBUG, ("etharp_query: sending ARP request.\n"));
...
...
-----------------

The other bugs are in dhcp.c. The dynamic address assignment works, but some
memory is lost "during the ride". The first location is line 965 and 966
(dhcp_unfold_reply()) where
dhcp->msg_in and dhcp->options_in are set to NULL without checking if they
already point to something (so if the dhcp_unfold_reply() gets called
multiple times, which can obviously happen, memory will be lost)

My solution is to replace line 965 and 966 with a call to dhcp_free_reply()
instead, so the first lines of the function should look like this:

-----------------
static err_t dhcp_unfold_reply(struct dhcp *dhcp)
{
  struct pbuf *p = dhcp->p;
  u8_t *ptr;
  u16_t i;
  u16_t j = 0;
  dhcp_free_reply(dhcp);
  /* options present? */
  if (dhcp->p->tot_len > sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN)
.....
.....
.....
-----------------

The dhcp_free_reply() should also be called from dhcp_stop(), to prevent
losing memory, I inserted a call on line 897, just before the dhcp object is
freed. The end of dhcp_stop() should look like this:

-----------------
....
....
    if (dhcp->p != NULL)
    {
      pbuf_free(dhcp->p);
      dhcp->p = NULL;
    }
    dhcp_free_reply(dhcp);
    mem_free((void *)dhcp);
    netif->dhcp = NULL;
  }
}
-----------------

There's also another thing I would like to point out: at some places, in
other parts the code, the lack of variable casting can cause problems with
some compilers. The locations are the following:
tcp_in.c, line 110
udp.c, line 174
icmp.c, line 64
At these three locations a minus sign (-) is prefixed to the second,
initially unsigned, argument for the pbuf_header()-functions in order to
make it negative. Most compilers handle this the "desired" way by performing
a unsigned->signed cast. However, the compiler I use (Keil C166) deals with
this in another way: it simply ignores the minus sign, if you don't do an
explicit cast first. So in order to make the code more compatible,
a -((s16_t)(...)) cast should be used, instead of just a minus sign...

Regards
Anders