Possible memory leak on dhcp server (IDFGH-4594)

[gmag11]

I'm running an project where ESP32 behaves as wifi station and AP. From time to time I get an entry on log like this:

dhcps: send_nak>>udp_sendto result 0

After that heap looses about 60 bytes that are never recovered.

As I'm using Arduino core I do not have access to dhcpserver.c code so that I'm not able to do any test on it. Please, check if code on https://github.com/espressif/esp-idf/blob/master/components/lwip/apps/dhcpserver/dhcpserver.c#L553 may cause this leak.

[david-cermak]

@gmag11 Thanks for the report, indeed there's a theoretical chance of allocating 20 bytes (struct dhcps_pool + struct _list_node) which were not free'd immediately after DHCP server replies with NAK. These structures are used to hold information about the client (that has requested another address than offered). The data are kept in the internal list of client information and reused when the same client (with the same MAC) reapplies for a lease. I think this could cause some memory leaks only if we repeatably request different IPs from clients that constantly change MACs.

[david-cermak]

I'm posting a patch which modifies the behavior. Could you please check if this the issue you're seeing? (perhaps you can use the arduino lib builder to test in with Arduino?)

---
 components/lwip/apps/dhcpserver/dhcpserver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/components/lwip/apps/dhcpserver/dhcpserver.c b/components/lwip/apps/dhcpserver/dhcpserver.c
index a50b3ccc78..f23966688f 100644
--- a/components/lwip/apps/dhcpserver/dhcpserver.c
+++ b/components/lwip/apps/dhcpserver/dhcpserver.c
@@ -916,7 +916,7 @@ POOL_CHECK:

         s16_t ret = parse_options(&m->options[4], len);;

-        if (ret == DHCPS_STATE_RELEASE) {
+        if (ret == DHCPS_STATE_RELEASE || ret == DHCPS_STATE_NAK) {
             if (pnode != NULL) {
                 node_remove_from_list(&plist, pnode);
                 free(pnode);
-- 

The current status seems to be in line with the RFC (https://tools.ietf.org/html/rfc2131#page-16):

A server MAY choose to mark addresses offered to clients in
DHCPOFFER messages as unavailable.

And at the same time, the record is stored and reused when needed, so for most usecases it just saves the heap from alloc/free cycles, and it doesn't in fact leak the memory, as there's a mechanism to discard leases.

[gmag11]

Hi, I'll try to setup Arduino lib builder although I've never used it.