Re: [PATCH net 7/7] selftests: ovpn: avoid calling freeaddrinfo() on NULL

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The Fixes: tag in commit "selftests: ovpn: avoid calling
  freeaddrinfo() on NULL" names the wrong commit.

--

selftests: ovpn: avoid calling freeaddrinfo() on NULL

This patch changes the exit path of ovpn_parse_remote() in
tools/testing/selftests/net/ovpn/ovpn-cli.c so that freeaddrinfo() is
called only when result is non-NULL. On musl, passing NULL to
freeaddrinfo() makes ovpn-cli segfault.

> ovpn_parse_remote() always calls freeaddrinfo() on its exit path, but
> result is NULL when no vpnip was passed (with or without a remote host).

[ ... ]

> Fixes: 959bc330a439 ("testing/selftests: add test tool and scripts for ovpn module")
> Cc: stable@vger.kernel.org

[Severity: Low]
Does this Fixes: tag point at the right commit?

In 959bc330a439, result had no initializer, and the host branch in
ovpn_parse_remote() did not free or reset it:

    struct addrinfo *result;
    ...
        memcpy(&ovpn->remote, result->ai_addr, result->ai_addrlen);
    }

So with a host and no vpnip, the out: label freed the valid list that
getaddrinfo() returned. The CMD_CONNECT path in main() is one such caller:

    ret = ovpn_parse_new_peer(ovpn, argv[3], argv[4], argv[5],
                              argv[6], NULL);

With no host and no vpnip, out: received an uninitialized pointer, not
NULL.

The NULL case looks like it came from commit 0bd9cfebc1c9
("selftests/net: ovpn: fix getaddrinfo memory leak in
ovpn_parse_remote()"). That commit added both of these:

    struct addrinfo *result = NULL;
    ...
        freeaddrinfo(result);
        result = NULL;

The commit message of 0bd9cfebc1c9 also describes the no-host, no-vpnip
case in 959bc330a439 as an uninitialized pointer.

So the statement "result is NULL when no vpnip was passed (with or
without a remote host)" only holds once 0bd9cfebc1c9 is applied. Should
the tag be this instead?

    Fixes: 0bd9cfebc1c9 ("selftests/net: ovpn: fix getaddrinfo memory leak in ovpn_parse_remote()")

The context of the modified hunk has not changed since 959bc330a439. With
Cc: stable, the patch would also apply cleanly to stable trees that lack
0bd9cfebc1c9.

In those trees, wouldn't the new if (result) check test an uninitialized
pointer on the no-host, no-vpnip path, so the patch fixes nothing there?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022121606.732029-2-antonio%40openvpn.net