[lwip-users] [lwip] Memory leak and queued packets in etharp.c

[Martin Glunz]

This is a multi-part message in MIME format.
--------------F381E81D0C9C72D0273BCF7D
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable

Hello

I=B4ve found a memory leak in the etharp.c module:
When etharp_output() returns an ARP request instead of
the original packet, the pbuf containing the request
will not be freed. My fix is the insertion of a new
function etharp_arp_free() that should be called after
the packet is sent.

There is another problem in etharp.c:
If a packet gets queued because an ARP request must be sent,
pbuf_ref() is called for this packet. This is ok so far, but
if the queued pbuf is chained, the chain may get lost later,
but before the packet is resent. I=B4ve fixed this in =

pbuf_dechain() in pbuf.c.

Attached the diff to the cvs source from 2002/10/31


Martin Glunz

WANTED: Schr=F6dinger's Cat. Dead or Alive.

There are only 10 kinds of people. Those who
understand binary and those who don't.
--------------F381E81D0C9C72D0273BCF7D
Content-Type: text/plain; charset=us-ascii;
 name="lwip-mg1.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="lwip-mg1.diff"

Only in lwip.orig/: CVS
diff -r -u lwip.orig/src/core/pbuf.c lwip.new/src/core/pbuf.c
--- lwip.orig/src/core/pbuf.c   Sat Oct 19 15:00:24 2002
+++ lwip.new/src/core/pbuf.c    Thu Oct 31 08:26:14 2002
@@ -585,13 +585,13 @@
 pbuf_dechain(struct pbuf *p)
 {
   struct pbuf *q;
-  
+
   q = p->next;
   if (q != NULL) {
     q->tot_len = p->tot_len - p->len;
   }
   p->tot_len = p->len;
-  p->next = NULL;
+  if (p->ref<=1) p->next = NULL;
   return q;
 }
 
/*-----------------------------------------------------------------------------------*/
diff -r -u lwip.orig/src/include/netif/etharp.h 
lwip.new/src/include/netif/etharp.h
--- lwip.orig/src/include/netif/etharp.h        Sat Oct 19 15:00:14 2002
+++ lwip.new/src/include/netif/etharp.h Thu Oct 31 08:22:12 2002
@@ -107,5 +107,10 @@
 struct pbuf *etharp_output(struct netif *netif, struct ip_addr *ipaddr,
                           struct pbuf *q);
 
+/* The etharp_arp_free() function must be called after a packet
+returned by etharp_output() is sent. It frees the pbuf if the
+packet was an ARP request.
+*/
+struct pbuf * etharp_arp_free(struct pbuf *p);
 
 #endif /* __NETIF_ARP_H__ */
diff -r -u lwip.orig/src/netif/etharp.c lwip.new/src/netif/etharp.c
--- lwip.orig/src/netif/etharp.c        Sat Oct 19 15:00:16 2002
+++ lwip.new/src/netif/etharp.c Thu Oct 31 08:54:12 2002
@@ -18,7 +18,7 @@
  * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO 
EVENT 
  * SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, 
  * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, 
PROCUREMENT 
- * OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS 
+ * OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
  * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN 
  * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) 
ARISING 
  * IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE 
POSSIBILITY 
@@ -91,8 +91,8 @@
   struct eth_addr ethaddr;
   enum etharp_state state;
   struct pbuf *p;
-  void *payload;
-  u16_t len, tot_len;
+  //void *payload;
+  //u16_t len, tot_len;
   u8_t ctime;
 };
 
@@ -193,14 +193,16 @@
        arp_table[i].ctime = ctime;
        arp_table[i].state = ETHARP_STATE_STABLE;
        p = arp_table[i].p;
-       if(p != NULL) { 
-         p->payload = arp_table[i].payload;    
-         p->len = arp_table[i].len;
-         p->tot_len = arp_table[i].tot_len;      
-         arp_table[i].p = NULL;        
-         
+       if(p != NULL) {
+         /* To do:
+           Handling of more than one queued packet */
+         //p->payload = arp_table[i].payload;
+         //p->len = arp_table[i].len;
+         //p->tot_len = arp_table[i].tot_len;
+         arp_table[i].p = NULL;
+
          ethhdr = p->payload;
-         
+
          for(k = 0; k < 6; ++k) {
            ethhdr->dest.addr[k] = ethaddr->addr[k];
          }
@@ -286,7 +288,7 @@
       return p;
     }
     break;
-  case ARP_REPLY:    
+  case ARP_REPLY:
     /* ARP reply. We insert or update the ARP table. */
     DEBUGF(ETHARP_DEBUG, ("etharp_arp_input: ARP reply\n"));
     if(ip_addr_cmp(&(hdr->dipaddr), &(netif->ip_addr))) {     
@@ -322,7 +324,7 @@
   srcaddr = (struct eth_addr *)netif->hwaddr;
 
   /* Make room for Ethernet header. */
-  if(pbuf_header(q, sizeof(struct eth_hdr)) != 0) {    
+  if(pbuf_header(q, sizeof(struct eth_hdr)) != 0) {
     /* The pbuf_header() call shouldn't fail, and we'll just bail
        out if it does.. */
     DEBUGF(ETHARP_DEBUG, ("etharp_output: could not allocate room for 
header.\n"));
@@ -353,12 +355,12 @@
   } else {
     if(!ip_addr_maskcmp(ipaddr, &(netif->ip_addr), &(netif->netmask))) {
       /* Use the IP address of the default gateway if the destination
-         is on the same subnet as we are. */      
+        is on the same subnet as we are. */
       ipaddr = &(netif->gw);
     }
 
     /* We try to find a stable mapping. */
-    for(i = 0; i < ARP_TABLE_SIZE; ++i) {    
+    for(i = 0; i < ARP_TABLE_SIZE; ++i) {
       if(arp_table[i].state == ETHARP_STATE_STABLE &&
         ip_addr_cmp(ipaddr, &arp_table[i].ipaddr)) {
        dest = &arp_table[i].ethaddr;
@@ -366,12 +368,14 @@
       }
     }
   }
-  
+
   if(dest == NULL) {
     /* No destination address has been found, so we'll have to send
        out an ARP request for the IP address. The outgoing packet is
        queued unless the queue is full. */
-    
+    /* To do:
+       Handling of more than one packet to be queued */
+
     /* We check if we are already querying for this address. If so,
        we'll bail out. */
     for(i = 0; i < ARP_TABLE_SIZE; ++i) {
@@ -383,13 +387,13 @@
     }
 
     i = find_arp_entry();
-    
+
     /* If all table entries were in pending state, we won't send out any
        more ARP requests. We'll just give up. */
     if(i == ARP_TABLE_SIZE) {
       return NULL;
     }
-    
+
     /* Now, i is the ARP table entry which we will fill with the new
        information. */
     ip_addr_set(&arp_table[i].ipaddr, ipaddr);
@@ -400,10 +404,10 @@
     arp_table[i].state = ETHARP_STATE_PENDING;
 #if 1
     arp_table[i].p = q;
-    arp_table[i].payload = q->payload;
-    arp_table[i].len = q->len;
-    arp_table[i].tot_len = q->tot_len;
-    
+    //arp_table[i].payload = q->payload;
+    //arp_table[i].len = q->len;
+    //arp_table[i].tot_len = q->tot_len;
+
     /* Because the pbuf will be queued, we'll increase the refernce
        count. */
     DEBUGF(ETHARP_DEBUG, ("etharp_output: queueing %p\n", q));
@@ -412,7 +416,7 @@
     arp_table[i].p = NULL;
 #endif /* 0 */
 
-    
+
     /* We allocate a pbuf for the outgoing ARP request packet. */
     p = pbuf_alloc(PBUF_LINK, sizeof(struct etharp_hdr), PBUF_RAM);
     if(p == NULL) {
@@ -424,22 +428,22 @@
           is dequeued). */
        DEBUGF(ETHARP_DEBUG, ("etharp_output: couldn't alloc pbuf for query, 
dequeueing %p\n", q));
        pbuf_free(q);
-      }      
+      }
       return NULL;
     }
-    
+
     hdr = p->payload;
     
     hdr->opcode = htons(ARP_REQUEST);
-    
+
     for(i = 0; i < 6; ++i) {
       hdr->dhwaddr.addr[i] = 0x00;
       hdr->shwaddr.addr[i] = srcaddr->addr[i];
     }
-    
+
     ip_addr_set(&(hdr->dipaddr), ipaddr);
     ip_addr_set(&(hdr->sipaddr), &(netif->ip_addr));
-    
+
     hdr->hwtype = htons(HWTYPE_ETHERNET);
     ARPH_HWLEN_SET(hdr, 6);
     
@@ -451,14 +455,15 @@
       hdr->ethhdr.src.addr[i] = srcaddr->addr[i];
     }
     
-    hdr->ethhdr.type = htons(ETHTYPE_ARP);      
+    hdr->ethhdr.type = htons(ETHTYPE_ARP);
+    pbuf_free(p);
     return p;
   } else {
     /* A valid IP->MAC address mapping was found, so we construct the
        Ethernet header for the outgoing packet. */
 
     ethhdr = q->payload;
-    
+
     for(i = 0; i < 6; i++) {
       ethhdr->dest.addr[i] = dest->addr[i];
       ethhdr->src.addr[i] = srcaddr->addr[i];
@@ -468,7 +473,18 @@
   
     return q;
   }
-  
+
 
 }
 
/*-----------------------------------------------------------------------------------*/
+
+struct pbuf *
+etharp_arp_free(struct pbuf *p)
+{
+  struct etharp_hdr *hdr;
+  hdr=p->payload;
+  if (hdr->opcode == htons(ARP_REQUEST)) {
+    pbuf_free(p); p=NULL;
+  };
+  return p;
+}

--------------F381E81D0C9C72D0273BCF7D--

[This message was sent through the lwip discussion list.]