[PATCH v2 3/3] net: lwip: dns: free the callback context after an aborted lookup

do_dns() keeps its dns_cb_arg on the stack and registers its address as
the dns_gethostbyname() callback argument. lwIP has no way to cancel a
pending lookup, so if the command is interrupted or times out while the
name is still resolving, dns_cb() fires later and writes the result into
the stack frame that has since been reused.

Allocate dns_cb_arg on the heap and add an 'abandoned' flag. If the
lookup did not complete, hand ownership to dns_cb(), which frees the
context when it eventually fires; otherwise free it before returning.

Fixes: aedcfec9ed78 ("net: lwip: add dns command")
Signed-off-by: Shahriyar Jalayeri <[email protected]>
---
 net/lwip/dns.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/net/lwip/dns.c b/net/lwip/dns.c
index b620b0611d6..e93720963b8 100644
--- a/net/lwip/dns.c
+++ b/net/lwip/dns.c
@@ -4,6 +4,7 @@
 #include <command.h>
 #include <console.h>
 #include <env.h>
+#include <malloc.h>
 #include <lwip/dns.h>
 #include <lwip/timeouts.h>
 #include <net.h>
@@ -15,6 +16,7 @@
 struct dns_cb_arg {
 	ip_addr_t host_ipaddr;
 	bool done;
+	bool abandoned;
 };
 
 static void do_dns_tmr(void *arg)
@@ -26,6 +28,12 @@ static void dns_cb(const char *name, const ip_addr_t *ipaddr, void *arg)
 {
 	struct dns_cb_arg *dns_cb_arg = arg;
 
+	/* Late callback for an abandoned lookup: reclaim and stop. */
+	if (dns_cb_arg->abandoned) {
+		free(dns_cb_arg);
+		return;
+	}
+
 	dns_cb_arg->done = true;
 
 	if (!ipaddr)
@@ -36,7 +44,7 @@ static void dns_cb(const char *name, const ip_addr_t *ipaddr, void *arg)
 
 static int dns_loop(struct udevice *udev, const char *name, const char *var)
 {
-	struct dns_cb_arg dns_cb_arg = { };
+	struct dns_cb_arg *dns_cb_arg;
 	struct netif *netif;
 	const char *ipstr;
 	ip_addr_t ipaddr;
@@ -52,18 +60,22 @@ static int dns_loop(struct udevice *udev, const char *name, const char *var)
 		return CMD_RET_FAILURE;
 	}
 
-	dns_cb_arg.done = false;
+	dns_cb_arg = calloc(1, sizeof(*dns_cb_arg));
+	if (!dns_cb_arg) {
+		net_lwip_remove_netif(netif);
+		return CMD_RET_FAILURE;
+	}
 
-	ret = dns_gethostbyname(name, &ipaddr, dns_cb, &dns_cb_arg);
+	ret = dns_gethostbyname(name, &ipaddr, dns_cb, dns_cb_arg);
 
 	if (ret == ERR_OK) {
-		dns_cb(name, &ipaddr, &dns_cb_arg);
+		dns_cb(name, &ipaddr, dns_cb_arg);
 	} else if (ret == ERR_INPROGRESS) {
 		start = get_timer(0);
 		sys_timeout(DNS_RESEND_MS, do_dns_tmr, NULL);
 		do {
 			net_lwip_rx(udev, netif);
-			if (dns_cb_arg.done)
+			if (dns_cb_arg->done)
 				break;
 			if (ctrlc()) {
 				printf("\nAbort\n");
@@ -75,15 +87,27 @@ static int dns_loop(struct udevice *udev, const char *name, const char *var)
 
 	net_lwip_remove_netif(netif);
 
-	if (dns_cb_arg.done && !ip_addr_isany(&dns_cb_arg.host_ipaddr)) {
-		ipstr = ipaddr_ntoa(&dns_cb_arg.host_ipaddr);
+	/*
+	 * A started lookup that never completed may still be pending in lwIP
+	 * and reference dns_cb_arg. Hand ownership to dns_cb, which frees it
+	 * when it eventually fires, instead of freeing it here.
+	 */
+	if (ret == ERR_INPROGRESS && !dns_cb_arg->done) {
+		dns_cb_arg->abandoned = true;
+		return CMD_RET_FAILURE;
+	}
+
+	if (dns_cb_arg->done && !ip_addr_isany(&dns_cb_arg->host_ipaddr)) {
+		ipstr = ipaddr_ntoa(&dns_cb_arg->host_ipaddr);
 		if (var)
 			env_set(var, ipstr);
 		else
 			printf("%s\n", ipstr);
+		free(dns_cb_arg);
 		return CMD_RET_SUCCESS;
 	}
 
+	free(dns_cb_arg);
 	return CMD_RET_FAILURE;
 }
 

-- 
2.43.0