Fix BUG#11400 - don't corrupt existing pbuf chain when enqueuing multiple pbufs to a pending ARP request

CHANGELOG

@@ -42,6 +42,10 @@ HISTORY
 
   ++ Bug fixes:
 
+  2007-03-04 Simon Goldschmidt
+  * etharp.c, etharp.h, memp.c, memp.h, opt.h: Fix BUG#11400 - don't corrupt
+    existing pbuf chain when enqueuing multiple pbufs to a pending ARP request
+
   2007-03-03 Frédéric Bernon
   * udp.c: remove obsolete line "static struct udp_pcb *pcb_cache = NULL;"
     Its is static, and never used in udp.c except udp_init().

src/core/memp.c

@@ -45,6 +45,10 @@
 #include "lwip/sys.h"
 #include "lwip/stats.h"
 
+#if ARP_QUEUEING
+#include "netif/etharp.h"
+#endif
+
 struct memp {
   struct memp *next;
 };
@@ -63,6 +67,9 @@ static const u16_t memp_sizes[MEMP_MAX] = {
   MEM_ALIGN_SIZE(sizeof(struct netbuf)),
   MEM_ALIGN_SIZE(sizeof(struct netconn)),
   MEM_ALIGN_SIZE(sizeof(struct tcpip_msg)),
+#if ARP_QUEUEING
+  MEM_ALIGN_SIZE(sizeof(struct etharp_q_entry)),
+#endif
   MEM_ALIGN_SIZE(sizeof(struct sys_timeo))
 };
 
@@ -76,6 +83,9 @@ static const u16_t memp_num[MEMP_MAX] = {
   MEMP_NUM_NETBUF,
   MEMP_NUM_NETCONN,
   MEMP_NUM_TCPIP_MSG,
+#if ARP_QUEUEING
+  MEMP_NUM_ARP_QUEUE,
+#endif
   MEMP_NUM_SYS_TIMEOUT
 };
 
@@ -92,6 +102,9 @@ static u8_t memp_memory[MEM_ALIGNMENT - 1 +
   MEMP_TYPE_SIZE(MEMP_NUM_NETBUF, struct netbuf) +
   MEMP_TYPE_SIZE(MEMP_NUM_NETCONN, struct netconn) +
   MEMP_TYPE_SIZE(MEMP_NUM_TCPIP_MSG, struct tcpip_msg) +
+#if ARP_QUEUEING
+  MEMP_TYPE_SIZE(MEMP_NUM_TCPIP_MSG, struct etharp_q_entry) +
+#endif
   MEMP_TYPE_SIZE(MEMP_NUM_SYS_TIMEOUT, struct sys_timeo)];
 
 #if !SYS_LIGHTWEIGHT_PROT

src/include/lwip/memp.h

@@ -46,7 +46,9 @@ typedef enum {
   MEMP_NETBUF,
   MEMP_NETCONN,
   MEMP_TCPIP_MSG,
-
+#if ARP_QUEUEING
+  MEMP_ARP_QUEUE,
+#endif
   MEMP_SYS_TIMEOUT,
   
   MEMP_MAX

src/include/lwip/opt.h

@@ -107,6 +107,12 @@ a lot of data that needs to be copied, this should be set high. */
 #ifndef MEMP_NUM_TCP_SEG
 #define MEMP_NUM_TCP_SEG                16
 #endif
+/* MEMP_NUM_ARP_QUEUE: the number of simulateously queued outgoing
+   packets (pbufs) that are waiting for an ARP request (to resolve
+   their destination address) to finish. */
+#ifndef MEMP_NUM_ARP_QUEUE
+#define MEMP_NUM_ARP_QUEUE              30
+#endif
 /* MEMP_NUM_SYS_TIMEOUT: the number of simulateously active
    timeouts. */
 #ifndef MEMP_NUM_SYS_TIMEOUT

src/include/netif/etharp.h

@@ -113,6 +113,15 @@ PACK_STRUCT_END
 #define ETHTYPE_ARP 0x0806
 #define ETHTYPE_IP  0x0800
 
+#if ARP_QUEUEING
+/** struct for queueing outgoing packets for unknown address
+  * defined here to be accessed by memp.h
+  */
+struct etharp_q_entry {
+  struct etharp_q_entry *next;
+  struct pbuf *p;
+};
+#endif
 
 void etharp_init(void);
 void etharp_tmr(void);

src/netif/etharp.c

@@ -93,7 +93,7 @@ struct etharp_entry {
   /** 
    * Pointer to queue of pending outgoing packets on this ARP entry.
    */
-   struct pbuf *p;
+  struct etharp_q_entry *q;
 #endif
   struct ip_addr ipaddr;
   struct eth_addr ethaddr;
@@ -123,13 +123,29 @@ etharp_init(void)
   for(i = 0; i < ARP_TABLE_SIZE; ++i) {
     arp_table[i].state = ETHARP_STATE_EMPTY;
 #if ARP_QUEUEING
-    arp_table[i].p = NULL;
+    arp_table[i].q = NULL;
 #endif
     arp_table[i].ctime = 0;
     arp_table[i].netif = NULL;
   }
 }
 
+#if ARP_QUEUEING
+/** Free a complete queue of etharp entrys */
+static void free_etharp_q(struct etharp_q_entry *q)
+{
+  struct etharp_q_entry *r;
+  LWIP_ASSERT("q != NULL", q != NULL);
+  LWIP_ASSERT("q->p != NULL", q->p != NULL);
+  while(q) {
+    r = q;
+    q = q->next;
+    pbuf_free(r->p);
+    memp_free(MEMP_ARP_QUEUE, r);
+  }
+}
+#endif
+
 /**
  * Clears expired entries in the ARP table.
  *
@@ -158,7 +174,7 @@ etharp_tmr(void)
         LWIP_DEBUGF(ETHARP_DEBUG, ("etharp_timer: expired pending entry %"U16_F".\n", (u16_t)i));
         arp_table[i].state = ETHARP_STATE_EXPIRED;
 #if ARP_QUEUEING
-      } else if (arp_table[i].p != NULL) {
+      } else if (arp_table[i].q != NULL) {
         /* resend an ARP query here */
 #endif
       }
@@ -169,11 +185,11 @@ etharp_tmr(void)
       snmp_delete_arpidx_tree(arp_table[i].netif, &arp_table[i].ipaddr);
 #if ARP_QUEUEING
       /* and empty packet queue */
-      if (arp_table[i].p != NULL) {
+      if (arp_table[i].q != NULL) {
         /* remove all queued packets */
-        LWIP_DEBUGF(ETHARP_DEBUG, ("etharp_timer: freeing entry %"U16_F", packet queue %p.\n", (u16_t)i, (void *)(arp_table[i].p)));
+        LWIP_DEBUGF(ETHARP_DEBUG, ("etharp_timer: freeing entry %"U16_F", packet queue %p.\n", (u16_t)i, (void *)(arp_table[i].q)));
-        pbuf_free(arp_table[i].p);
+        free_etharp_q(arp_table[i].q);
-        arp_table[i].p = NULL;
+        arp_table[i].q = NULL;
       }
 #endif
       /* recycle entry for re-use */      
@@ -247,7 +263,7 @@ static s8_t find_entry(struct ip_addr *ipaddr, u8_t flags)
         return i;
 #if ARP_QUEUEING
       /* pending with queued packets? */
-      } else if (arp_table[i].p != NULL) {
+      } else if (arp_table[i].q != NULL) {
         if (arp_table[i].ctime >= age_queue) {
           old_queue = i;
           age_queue = arp_table[i].ctime;
@@ -304,7 +320,7 @@ static s8_t find_entry(struct ip_addr *ipaddr, u8_t flags)
     LWIP_DEBUGF(ETHARP_DEBUG | DBG_TRACE, ("find_entry: selecting oldest stable entry %"U16_F"\n", (u16_t)i));
 #if ARP_QUEUEING
     /* no queued packets should exist on stable entries */
-    LWIP_ASSERT("arp_table[i].p == NULL", arp_table[i].p == NULL);
+    LWIP_ASSERT("arp_table[i].q == NULL", arp_table[i].q == NULL);
 #endif
   /* 3) found recyclable pending entry without queued packets? */
   } else if (old_pending < ARP_TABLE_SIZE) {
@@ -316,9 +332,9 @@ static s8_t find_entry(struct ip_addr *ipaddr, u8_t flags)
   } else if (old_queue < ARP_TABLE_SIZE) {
     /* recycle oldest pending */
     i = old_queue;
-    LWIP_DEBUGF(ETHARP_DEBUG | DBG_TRACE, ("find_entry: selecting oldest pending entry %"U16_F", freeing packet queue %p\n", (u16_t)i, (void *)(arp_table[i].p)));
+    LWIP_DEBUGF(ETHARP_DEBUG | DBG_TRACE, ("find_entry: selecting oldest pending entry %"U16_F", freeing packet queue %p\n", (u16_t)i, (void *)(arp_table[i].q)));
-    pbuf_free(arp_table[i].p);
+    free_etharp_q(arp_table[i].q);
-    arp_table[i].p = NULL;
+    arp_table[i].q = NULL;
 #endif
     /* no empty or recyclable entries found */
   } else {
@@ -405,14 +421,19 @@ update_arp_entry(struct netif *netif, struct ip_addr *ipaddr, struct eth_addr *e
   arp_table[i].ctime = 0;
 /* this is where we will send out queued packets! */
 #if ARP_QUEUEING
-  while (arp_table[i].p != NULL) {
+  while (arp_table[i].q != NULL) {
-    /* get the first packet on the queue */
+    struct pbuf *p;
-    struct pbuf *p = arp_table[i].p;
+    struct eth_hdr *ethhdr;
+    /* remember remainder of queue */
+    struct etharp_q_entry *q = arp_table[i].q;
+    /* pop first item off the queue */
+    arp_table[i].q = q->next;
+    /* get the packet pointer */
+    p = q->p;
+    /* now queue entry can be freed */
+    memp_free(MEMP_ARP_QUEUE, q);
     /* Ethernet header */
-    struct eth_hdr *ethhdr = p->payload;
+    ethhdr = p->payload;
-    /* remember (and reference) remainder of queue */
-    /* note: this will also terminate the p pbuf chain */
-    arp_table[i].p = pbuf_dequeue(p);
     /* fill-in Ethernet header */
     k = netif->hwaddr_len;
     while(k > 0) {
@@ -810,19 +831,29 @@ err_t etharp_query(struct netif *netif, struct ip_addr *ipaddr, struct pbuf *q)
       /* copy any PBUF_REF referenced payloads into PBUF_RAM */
       /* (the caller of lwIP assumes the referenced payload can be
        * freed after it returns from the lwIP call that brought us here) */
+      /* @todo: if q->type == PBUF_REF, do we have a memory leak (since we call pbuf_ref(p) later)? */
       p = pbuf_take(q);
       /* packet could be taken over? */
       if (p != NULL) {
         /* queue packet ... */
-        if (arp_table[i].p == NULL) {
+        struct etharp_q_entry *newEntry;
-          /* ... in the empty queue */
+        /* allocate a new arp queue entry */
+        newEntry = memp_malloc(MEMP_ARP_QUEUE);
+        LWIP_ASSERT("newEntry != NULL", newEntry != NULL);
+        newEntry->next = 0;
+        newEntry->p = p;
         pbuf_ref(p);
-          arp_table[i].p = p;
+        if(arp_table[i].q != NULL) {
-#if 0 /* multi-packet-queueing disabled, see bug #11400 */
+          /* queue was already existent, append the new entry to the end */
+          struct etharp_q_entry *r;
+          r = arp_table[i].q;
+          while(r->next != NULL) {
+            r = r->next;
+          }
+          r->next = newEntry;
         } else {
-          /* ... at tail of non-empty queue */
+          /* queue did not exist, first item in queue */
-          pbuf_queue(arp_table[i].p, p);
+          arp_table[i].q = newEntry;
-#endif
         }
         LWIP_DEBUGF(ETHARP_DEBUG | DBG_TRACE, ("etharp_query: queued packet %p on ARP entry %"S16_F"\n", (void *)q, (s16_t)i));
         result = ERR_OK;