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

Hi Shahriyar,

On 13/08/2026 18:28, Shahriyar Jalayeri wrote:
> 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.

Similar to the wget case, this relies on a later DNS callback to reclaim
the heap context, but that callback is not guaranteed to run.

Here too I suggest cancelling the queued DNS callback before returning
from dns, while the stack object is still valid. With a small DNS
cancellation helper, dns_cb_arg can stay on the stack and no heap object
has to be left for a later callback to free.

Thanks,
-- 
Jerome

> 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;
>  }
>  
>