Rework DHCP PCB handling: Old code registered one UDP PCB per netif where DHCP was active and there was a special case in udp_input() for this. New implementation uses one PCB for all netifs and removes special case in udp_input().

The old approach called udp_bind() on each of the PCBs, which puts them into udp_pcbs list. The PCBs were iterated on all non-DHCP udp_inputs() with no effect.
My cleanup removes the special handling in udp.c, and uses only one DHCP UDP PCB to catch all DHCP messages from all netifs. The dhcp_recv function then checks whether ip_current_input_netif() has DHCP enabled - if not, the message is ignored. The PCB is only created/registered when one or more PCBs have DHCP enabled.

src/core/ipv4/dhcp.c

@@ -158,6 +158,8 @@ static u8_t xid_initialised;
 #define dhcp_get_option_value(dhcp, idx)      (dhcp_rx_options_val[idx])
 #define dhcp_set_option_value(dhcp, idx, val) (dhcp_rx_options_val[idx] = (val))
 
+static struct udp_pcb *dhcp_pcb;
+static u8_t dhcp_pcb_refcount;
 
 /* DHCP client state machine functions */
 static err_t dhcp_discover(struct netif *netif);
@@ -195,6 +197,46 @@ static void dhcp_option_hostname(struct dhcp *dhcp, struct netif *netif);
 /* always add the DHCP options trailer to end and pad */
 static void dhcp_option_trailer(struct dhcp *dhcp);
 
+/** Ensure DHCP PCB is allocated and bound */
+static err_t
+dhcp_inc_pcb_refcount(void)
+{
+  if(dhcp_pcb_refcount == 0) {
+    LWIP_ASSERT("dhcp_inc_pcb_refcount(): memory leak", dhcp_pcb == NULL);
+
+    /* allocate UDP PCB */
+    dhcp_pcb = udp_new();
+
+    if(dhcp_pcb == NULL) {
+      return ERR_MEM;
+    }
+
+    ip_set_option(dhcp_pcb, SOF_BROADCAST);
+
+    /* set up local and remote port for the pcb -> listen on all interfaces on all src/dest IPs */
+    udp_bind(dhcp_pcb, IP_ADDR_ANY, DHCP_CLIENT_PORT);
+    udp_connect(dhcp_pcb, IP_ADDR_ANY, DHCP_SERVER_PORT);
+    udp_recv(dhcp_pcb, dhcp_recv, NULL);  
+  }
+
+  dhcp_pcb_refcount++;
+  
+  return ERR_OK;
+}
+
+/** Free DHCP PCB if the last netif stops using it */
+static void
+dhcp_dec_pcb_refcount(void)
+{
+  LWIP_ASSERT("dhcp_pcb_refcount(): refcount error", (dhcp_pcb_refcount > 0));
+  dhcp_pcb_refcount--;
+  
+  if(dhcp_pcb_refcount == 0) {
+    udp_remove(dhcp_pcb);
+    dhcp_pcb = NULL;
+  }
+}
+
 /**
  * Back-off the DHCP client (because of a received NAK response).
  *
@@ -329,7 +371,7 @@ dhcp_select(struct netif *netif)
     pbuf_realloc(dhcp->p_out, sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN + dhcp->options_out_len);
 
     /* send broadcast to any DHCP server */
-    udp_sendto_if_src(dhcp->pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif, IP_ADDR_ANY);
+    udp_sendto_if_src(dhcp_pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif, IP_ADDR_ANY);
     dhcp_delete_msg(dhcp);
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_select: REQUESTING\n"));
   } else {
@@ -679,35 +721,32 @@ dhcp_start(struct netif *netif)
       LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_start(): could not allocate dhcp\n"));
       return ERR_MEM;
     }
+    
     /* store this dhcp client in the netif */
     netif->dhcp = dhcp;
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_start(): allocated dhcp"));
   /* already has DHCP client attached */
   } else {
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_start(): restarting DHCP configuration\n"));
-    if (dhcp->pcb != NULL) {
-      udp_remove(dhcp->pcb);
-    }
     LWIP_ASSERT("pbuf p_out wasn't freed", dhcp->p_out == NULL);
     LWIP_ASSERT("reply wasn't freed", dhcp->msg_in == NULL );
+
+    if(dhcp->pcb_allocated != 0) {
+      dhcp_dec_pcb_refcount(); /* free DHCP PCB if not needed any more */
+    }
+    /* dhcp is cleared below, no need to reset flag*/
   }
 
   /* clear data structure */
   memset(dhcp, 0, sizeof(struct dhcp));
   /* dhcp_set_state(&dhcp, DHCP_STATE_OFF); */
-  /* allocate UDP PCB */
+
-  dhcp->pcb = udp_new();
+  LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_start(): starting DHCP configuration\n"));  
-  if (dhcp->pcb == NULL) {
+
-    LWIP_DEBUGF(DHCP_DEBUG  | LWIP_DBG_TRACE, ("dhcp_start(): could not obtain pcb\n"));
+  if(dhcp_inc_pcb_refcount() != ERR_OK) { /* ensure DHCP PCB is allocated */
     return ERR_MEM;
   }
-  ip_set_option(dhcp->pcb, SOF_BROADCAST);
+  dhcp->pcb_allocated = 1;
-  /* set up local and remote port for the pcb */
-  udp_bind(dhcp->pcb, IP_ADDR_ANY, DHCP_CLIENT_PORT);
-  udp_connect(dhcp->pcb, IP_ADDR_ANY, DHCP_SERVER_PORT);
-  /* set up the recv callback and argument */
-  udp_recv(dhcp->pcb, dhcp_recv, netif);
-  LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_start(): starting DHCP configuration\n"));
 
 #if LWIP_DHCP_CHECK_LINK_UP
   if (!netif_is_link_up(netif)) {
@@ -717,6 +756,7 @@ dhcp_start(struct netif *netif)
   }
 #endif /* LWIP_DHCP_CHECK_LINK_UP */
 
+
   /* (re)start the DHCP negotiation */
   result = dhcp_discover(netif);
   if (result != ERR_OK) {
@@ -741,27 +781,16 @@ dhcp_inform(struct netif *netif)
 {
   struct dhcp dhcp;
   err_t result = ERR_OK;
-  struct udp_pcb *pcb;
 
   LWIP_ERROR("netif != NULL", (netif != NULL), return;);
 
+  if(dhcp_inc_pcb_refcount() != ERR_OK) { /* ensure DHCP PCB is allocated */
+    return;
+  }
+
   memset(&dhcp, 0, sizeof(struct dhcp));
   dhcp_set_state(&dhcp, DHCP_STATE_INFORMING);
 
-  if ((netif->dhcp != NULL) && (netif->dhcp->pcb != NULL)) {
-    /* re-use existing pcb */
-    pcb = netif->dhcp->pcb;
-  } else {
-    pcb = udp_new();
-    if (pcb == NULL) {
-      LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_LEVEL_SERIOUS, ("dhcp_inform(): could not obtain pcb"));
-      return;
-    }
-    dhcp.pcb = pcb;
-    ip_set_option(dhcp.pcb, SOF_BROADCAST);
-    udp_bind(dhcp.pcb, IP_ADDR_ANY, DHCP_CLIENT_PORT);
-    LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_inform(): created new udp pcb\n"));
-  }
   /* create and initialize the DHCP message header */
   result = dhcp_create_msg(netif, &dhcp, DHCP_INFORM);
   if (result == ERR_OK) {
@@ -773,16 +802,15 @@ dhcp_inform(struct netif *netif)
     pbuf_realloc(dhcp.p_out, sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN + dhcp.options_out_len);
 
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_inform: INFORMING\n"));
-    udp_sendto_if(pcb, dhcp.p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif);
+    
+    udp_sendto_if(dhcp_pcb, dhcp.p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif);
+
     dhcp_delete_msg(&dhcp);
   } else {
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_LEVEL_SERIOUS, ("dhcp_inform: could not allocate DHCP request\n"));
   }
 
-  if (dhcp.pcb != NULL) {
+  dhcp_dec_pcb_refcount(); /* delete DHCP PCB if not needed any more */
-    /* otherwise, the existing pcb was used */
-    udp_remove(dhcp.pcb);
-  }
 }
 
 /** Handle a possible change in the network configuration.
@@ -878,7 +906,7 @@ dhcp_decline(struct netif *netif)
     pbuf_realloc(dhcp->p_out, sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN + dhcp->options_out_len);
 
     /* per section 4.4.4, broadcast DECLINE messages */
-    udp_sendto_if_src(dhcp->pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif, IP_ADDR_ANY);
+    udp_sendto_if_src(dhcp_pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif, IP_ADDR_ANY);
     dhcp_delete_msg(dhcp);
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_decline: BACKING OFF\n"));
   } else {
@@ -929,7 +957,7 @@ dhcp_discover(struct netif *netif)
     pbuf_realloc(dhcp->p_out, sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN + dhcp->options_out_len);
 
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_discover: sendto(DISCOVER, IP_ADDR_BROADCAST, DHCP_SERVER_PORT)\n"));
-    udp_sendto_if_src(dhcp->pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif, IP_ADDR_ANY);
+    udp_sendto_if_src(dhcp_pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif, IP_ADDR_ANY);
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_discover: deleting()ing\n"));
     dhcp_delete_msg(dhcp);
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_discover: SELECTING\n"));
@@ -1089,7 +1117,7 @@ dhcp_renew(struct netif *netif)
 
     pbuf_realloc(dhcp->p_out, sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN + dhcp->options_out_len);
 
-    udp_sendto_if(dhcp->pcb, dhcp->p_out, &dhcp->server_ip_addr, DHCP_SERVER_PORT, netif);
+    udp_sendto_if(dhcp_pcb, dhcp->p_out, &dhcp->server_ip_addr, DHCP_SERVER_PORT, netif);
     dhcp_delete_msg(dhcp);
 
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_renew: RENEWING\n"));
@@ -1135,7 +1163,7 @@ dhcp_rebind(struct netif *netif)
     pbuf_realloc(dhcp->p_out, sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN + dhcp->options_out_len);
 
     /* broadcast to server */
-    udp_sendto_if(dhcp->pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif);
+    udp_sendto_if(dhcp_pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif);
     dhcp_delete_msg(dhcp);
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_rebind: REBINDING\n"));
   } else {
@@ -1178,7 +1206,7 @@ dhcp_reboot(struct netif *netif)
     pbuf_realloc(dhcp->p_out, sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN + dhcp->options_out_len);
 
     /* broadcast to server */
-    udp_sendto_if(dhcp->pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif);
+    udp_sendto_if(dhcp_pcb, dhcp->p_out, IP_ADDR_BROADCAST, DHCP_SERVER_PORT, netif);
     dhcp_delete_msg(dhcp);
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_reboot: REBOOTING\n"));
   } else {
@@ -1243,7 +1271,7 @@ dhcp_release(struct netif *netif)
 
     pbuf_realloc(dhcp->p_out, sizeof(struct dhcp_msg) - DHCP_OPTIONS_LEN + dhcp->options_out_len);
 
-    udp_sendto_if(dhcp->pcb, dhcp->p_out, &server_ip_addr, DHCP_SERVER_PORT, netif);
+    udp_sendto_if(dhcp_pcb, dhcp->p_out, &server_ip_addr, DHCP_SERVER_PORT, netif);
     dhcp_delete_msg(dhcp);
     LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE | LWIP_DBG_STATE, ("dhcp_release: RELEASED, DHCP_STATE_OFF\n"));
   } else {
@@ -1278,12 +1306,13 @@ dhcp_stop(struct netif *netif)
     }
 #endif /* LWIP_DHCP_AUTOIP_COOP */
 
-    if (dhcp->pcb != NULL) {
-      udp_remove(dhcp->pcb);
-      dhcp->pcb = NULL;
-    }
     LWIP_ASSERT("reply wasn't freed", dhcp->msg_in == NULL);
     dhcp_set_state(dhcp, DHCP_STATE_OFF);
+
+    if(dhcp->pcb_allocated != 0) {
+      dhcp_dec_pcb_refcount(); /* free DHCP PCB if not needed any more */
+      dhcp->pcb_allocated = 0;
+    }
   }
 }
 
@@ -1589,16 +1618,21 @@ decode_next:
 static void
 dhcp_recv(void *arg, struct udp_pcb *pcb, struct pbuf *p, const ip_addr_t *addr, u16_t port)
 {
-  struct netif *netif = (struct netif *)arg;
+  struct netif *netif = ip_current_input_netif();
   struct dhcp *dhcp = netif->dhcp;
   struct dhcp_msg *reply_msg = (struct dhcp_msg *)p->payload;
   u8_t msg_type;
   u8_t i;
+  
+  LWIP_UNUSED_ARG(arg);
+
+  /* Caught DHCP message from netif that does not have DHCP enabled? -> not interested */
+  if((dhcp == NULL) || (dhcp->pcb_allocated == 0)) {
+    goto free_pbuf_and_return;
+  }
 
-#if LWIP_IPV6
   LWIP_ASSERT("invalid server address type", !IP_IS_V6(addr));
-#endif /* LWIP_IPV6 */
+  
-
   LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("dhcp_recv(pbuf = %p) from DHCP server %"U16_F".%"U16_F".%"U16_F".%"U16_F" port %"U16_F"\n", (void*)p,
     ip4_addr1_16(ip_2_ip4(addr)), ip4_addr2_16(ip_2_ip4(addr)), ip4_addr3_16(ip_2_ip4(addr)), ip4_addr4_16(ip_2_ip4(addr)), port));
   LWIP_DEBUGF(DHCP_DEBUG | LWIP_DBG_TRACE, ("pbuf->len = %"U16_F"\n", p->len));

src/core/udp.c

@@ -260,74 +260,53 @@ udp_input(struct pbuf *p, struct netif *inp)
   ip_addr_debug_print(UDP_DEBUG, ip_current_src_addr());
   LWIP_DEBUGF(UDP_DEBUG, (", %"U16_F")\n", ntohs(udphdr->src)));
 
-#if LWIP_DHCP
   pcb = NULL;
-  /* when LWIP_DHCP is active, packets to DHCP_CLIENT_PORT may only be processed by
+  prev = NULL;
-     the dhcp module, no other UDP pcb may use the local UDP port DHCP_CLIENT_PORT */
+  uncon_pcb = NULL;
-  if (dest == DHCP_CLIENT_PORT) {
+  /* Iterate through the UDP pcb list for a matching pcb.
-    /* all packets for DHCP_CLIENT_PORT not coming from DHCP_SERVER_PORT are dropped! */
+   * 'Perfect match' pcbs (connected to the remote port & ip address) are
-    if (src == DHCP_SERVER_PORT) {
+   * preferred. If no perfect match is found, the first unconnected pcb that
-      if ((inp->dhcp != NULL) && (inp->dhcp->pcb != NULL)) {
+   * matches the local port and ip address gets the datagram. */
-        /* accept the packet if
+  for (pcb = udp_pcbs; pcb != NULL; pcb = pcb->next) {
-           (- broadcast or directed to us) -> DHCP is link-layer-addressed, local ip is always ANY!
+    /* print the PCB local and remote address */
-           - inp->dhcp->pcb->remote == ANY or iphdr->src
+    LWIP_DEBUGF(UDP_DEBUG, ("pcb ("));
-           (no need to check for IPv6 since the dhcp struct always uses IPv4) */
+    ip_addr_debug_print(UDP_DEBUG, &pcb->local_ip);
-        if (ip_addr_isany_val(inp->dhcp->pcb->remote_ip) ||
+    LWIP_DEBUGF(UDP_DEBUG, (", %"U16_F") <-- (", pcb->local_port));
-            ip_addr_cmp(&inp->dhcp->pcb->remote_ip, ip_current_src_addr())) {
+    ip_addr_debug_print(UDP_DEBUG, &pcb->remote_ip);
-          pcb = inp->dhcp->pcb;
+    LWIP_DEBUGF(UDP_DEBUG, (", %"U16_F")\n", pcb->remote_port));
-        }
-      }
-    }
-  } else
-#endif /* LWIP_DHCP */
-  {
-    prev = NULL;
-    uncon_pcb = NULL;
-    /* Iterate through the UDP pcb list for a matching pcb.
-     * 'Perfect match' pcbs (connected to the remote port & ip address) are
-     * preferred. If no perfect match is found, the first unconnected pcb that
-     * matches the local port and ip address gets the datagram. */
-    for (pcb = udp_pcbs; pcb != NULL; pcb = pcb->next) {
-      /* print the PCB local and remote address */
-      LWIP_DEBUGF(UDP_DEBUG, ("pcb ("));
-      ip_addr_debug_print(UDP_DEBUG, &pcb->local_ip);
-      LWIP_DEBUGF(UDP_DEBUG, (", %"U16_F") <-- (", pcb->local_port));
-      ip_addr_debug_print(UDP_DEBUG, &pcb->remote_ip);
-      LWIP_DEBUGF(UDP_DEBUG, (", %"U16_F")\n", pcb->remote_port));
 
-      /* compare PCB local addr+port to UDP destination addr+port */
+    /* compare PCB local addr+port to UDP destination addr+port */
-      if ((pcb->local_port == dest) &&
+    if ((pcb->local_port == dest) &&
-          (udp_input_local_match(pcb, inp, broadcast) != 0)) {
+        (udp_input_local_match(pcb, inp, broadcast) != 0)) {
-        if ((uncon_pcb == NULL) &&
+      if ((uncon_pcb == NULL) &&
-            ((pcb->flags & UDP_FLAGS_CONNECTED) == 0)) {
+          ((pcb->flags & UDP_FLAGS_CONNECTED) == 0)) {
-          /* the first unconnected matching PCB */
+        /* the first unconnected matching PCB */
-          uncon_pcb = pcb;
+        uncon_pcb = pcb;
-        }
-
-        /* compare PCB remote addr+port to UDP source addr+port */
-        if ((pcb->remote_port == src) &&
-            (ip_addr_isany_val(pcb->remote_ip) ||
-            ip_addr_cmp(&pcb->remote_ip, ip_current_src_addr()))) {
-          /* the first fully matching PCB */
-          if (prev != NULL) {
-            /* move the pcb to the front of udp_pcbs so that is
-               found faster next time */
-            prev->next = pcb->next;
-            pcb->next = udp_pcbs;
-            udp_pcbs = pcb;
-          } else {
-            UDP_STATS_INC(udp.cachehit);
-          }
-          break;
-        }
       }
 
-      prev = pcb;
+      /* compare PCB remote addr+port to UDP source addr+port */
-    }
+      if ((pcb->remote_port == src) &&
-    /* no fully matching pcb found? then look for an unconnected pcb */
+          (ip_addr_isany_val(pcb->remote_ip) ||
-    if (pcb == NULL) {
+          ip_addr_cmp(&pcb->remote_ip, ip_current_src_addr()))) {
-      pcb = uncon_pcb;
+        /* the first fully matching PCB */
+        if (prev != NULL) {
+          /* move the pcb to the front of udp_pcbs so that is
+             found faster next time */
+          prev->next = pcb->next;
+          pcb->next = udp_pcbs;
+          udp_pcbs = pcb;
+        } else {
+          UDP_STATS_INC(udp.cachehit);
+        }
+        break;
+      }
     }
+
+    prev = pcb;
+  }
+  /* no fully matching pcb found? then look for an unconnected pcb */
+  if (pcb == NULL) {
+    pcb = uncon_pcb;
   }
 
   /* Check checksum if this is a match or if it was directed at us. */

src/include/lwip/dhcp.h

@@ -59,10 +59,10 @@ struct dhcp
 {
   /** transaction identifier of last sent request */
   u32_t xid;
-  /** our connection to the DHCP server */
-  struct udp_pcb *pcb;
   /** incoming msg */
   struct dhcp_msg *msg_in;
+  /** track PCB allocation state */
+  u8_t pcb_allocated;
   /** current DHCP state machine state */
   u8_t state;
   /** retries of current request */