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