[PATCH v2 1/3] net: lwip: wget: fix error handling in wget_do_request()

[Ilias Apalodimas]

Hi David,

On Wed, 13 May 2026 at 00:48, David Lechner <dlechner at baylibre.com> wrote:
>
> Split wget_do_request() into two functions to make error handling less
> error-prone.
>
> After a successful call to net_lwip_new_netif(), net_lwip_remove_netif()
> must always be called to leaks.

prevent leaks*

> This was missed in the HTTPS section of
> the code where we returned on error without cleaning up.

I'd prefer making this a bit more explicit. net_lwip_remove_netif()
was called in case of a failure afaict. The only place that wasn's
called is when checking the cacert_auth_mode, right?

Anyway, I think Jerome can fix them up during the PR

Acked-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>

Thanks
/Ilias

>
> Instead of adding more calls to net_lwip_remove_netif(), refactor the
> code into two functions. The outer function handles managing the netif
> lifecycle. The inner function no longer has to worry about cleaning up
> before returning on error.
>
> To keep things simple, the `path` local variable is removed during the
> refactoring. Instead, ctx.path is used directly everywhere.
>
> Fixes: 3c656c928bd7 ("net: lwip: add wget command")
> Signed-off-by: David Lechner <dlechner at baylibre.com>
> ---
>  net/lwip/wget.c | 88 ++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 47 insertions(+), 41 deletions(-)
>
> diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> index 502c0faebb2..f89a6580b41 100644
> --- a/net/lwip/wget.c
> +++ b/net/lwip/wget.c
> @@ -292,45 +292,17 @@ static err_t httpc_headers_done_cb(httpc_state_t *connection, void *arg, struct
>  #if CONFIG_IS_ENABLED(WGET_CACERT)
>  #endif
>
> -int wget_do_request(ulong dst_addr, char *uri)
> +static int wget_handle_request(struct wget_ctx *ctx, bool is_https,
> +                              struct udevice *udev, struct netif *netif)
>  {
>  #if CONFIG_IS_ENABLED(WGET_HTTPS)
>         altcp_allocator_t tls_allocator;
>  #endif
>         httpc_connection_t conn;
>         httpc_state_t *state;
> -       struct udevice *udev;
> -       struct netif *netif;
> -       struct wget_ctx ctx;
> -       char *path;
> -       bool is_https;
> -
> -       ctx.daddr = dst_addr;
> -       ctx.saved_daddr = dst_addr;
> -       ctx.done = NOT_DONE;
> -       ctx.size = 0;
> -       ctx.prevsize = 0;
> -       ctx.start_time = 0;
> -       ctx.content_len = 0;
> -       ctx.hash_count = 0;
> -
> -       if (parse_url(uri, ctx.server_name, &ctx.port, &path, &is_https))
> -               return CMD_RET_USAGE;
> -
> -       if (net_lwip_eth_start() < 0)
> -               return CMD_RET_FAILURE;
> -
> -       if (!wget_info)
> -               wget_info = &default_wget_info;
> -
> -       udev = eth_get_dev();
> -
> -       netif = net_lwip_new_netif(udev);
> -       if (!netif)
> -               return -1;
>
>         /* if URL with hostname init dns */
> -       if (!ipaddr_aton(ctx.server_name, NULL) && net_lwip_dns_init())
> +       if (!ipaddr_aton(ctx->server_name, NULL) && net_lwip_dns_init())
>                 return CMD_RET_FAILURE;
>
>         memset(&conn, 0, sizeof(conn));
> @@ -374,11 +346,10 @@ int wget_do_request(ulong dst_addr, char *uri)
>                 tls_allocator.alloc = &altcp_tls_alloc;
>                 tls_allocator.arg =
>                         altcp_tls_create_config_client(ca, ca_sz,
> -                                                      ctx.server_name);
> +                                                      ctx->server_name);
>
>                 if (!tls_allocator.arg) {
>                         log_err("error: Cannot create a TLS connection\n");
> -                       net_lwip_remove_netif(netif);
>                         return -1;
>                 }
>
> @@ -388,24 +359,20 @@ int wget_do_request(ulong dst_addr, char *uri)
>
>         conn.result_fn = httpc_result_cb;
>         conn.headers_done_fn = httpc_headers_done_cb;
> -       ctx.path = path;
> -       if (httpc_get_file_dns(ctx.server_name, ctx.port, path, &conn, httpc_recv_cb,
> -                              &ctx, &state)) {
> -               net_lwip_remove_netif(netif);
> +       if (httpc_get_file_dns(ctx->server_name, ctx->port, ctx->path, &conn,
> +                              httpc_recv_cb, ctx, &state)) {
>                 return CMD_RET_FAILURE;
>         }
>
>         errno = 0;
>
> -       while (!ctx.done) {
> +       while (!ctx->done) {
>                 net_lwip_rx(udev, netif);
>                 if (ctrlc())
>                         break;
>         }
>
> -       net_lwip_remove_netif(netif);
> -
> -       if (ctx.done == SUCCESS)
> +       if (ctx->done == SUCCESS)
>                 return 0;
>
>         if (errno == EPERM && !wget_info->silent)
> @@ -414,6 +381,45 @@ int wget_do_request(ulong dst_addr, char *uri)
>         return -1;
>  }
>
> +int wget_do_request(ulong dst_addr, char *uri)
> +{
> +       struct udevice *udev;
> +       struct wget_ctx ctx;
> +       struct netif *netif;
> +       bool is_https;
> +       int ret;
> +
> +       ctx.daddr = dst_addr;
> +       ctx.saved_daddr = dst_addr;
> +       ctx.done = NOT_DONE;
> +       ctx.size = 0;
> +       ctx.prevsize = 0;
> +       ctx.start_time = 0;
> +       ctx.content_len = 0;
> +       ctx.hash_count = 0;
> +
> +       if (parse_url(uri, ctx.server_name, &ctx.port, &ctx.path, &is_https))
> +               return CMD_RET_USAGE;
> +
> +       if (net_lwip_eth_start() < 0)
> +               return CMD_RET_FAILURE;
> +
> +       if (!wget_info)
> +               wget_info = &default_wget_info;
> +
> +       udev = eth_get_dev();
> +
> +       netif = net_lwip_new_netif(udev);
> +       if (!netif)
> +               return -1;
> +
> +       ret = wget_handle_request(&ctx, is_https, udev, netif);
> +
> +       net_lwip_remove_netif(netif);
> +
> +       return ret;
> +}
> +
>  /**
>   * wget_validate_uri() - validate the uri for wget
>   *
>
> --
> 2.43.0
>