netdev - Re: [PATCH net v4] selftest:net: Fix uninit return values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a21498ff-4dd0-4b3a-9b2e-9b932b5925ad@openvpn.net>
Date: Tue, 30 Sep 2025 13:20:02 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sidharth Seela <sidharthseela@...il.com>, sd@...asysnail.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
 shuah@...nel.org, willemdebruijn.kernel@...il.com, kernelxing@...cent.com,
 nathan@...nel.org, nick.desaulniers+lkml@...il.com, morbo@...gle.com,
 justinstitt@...gle.com
Cc: netdev@...r.kernel.org, linux-kselftest@...r.kernel.org,
 linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH net v4] selftest:net: Fix uninit return values

Hi,

Thanks a lot for fixing this - I hadn't see the warnings with gcc.

On 30/09/2025 12:06, Sidharth Seela wrote:
> Fix functions that return undefined values. These issues were caught by
> running clang using LLVM=1 option.
> 
> 
> Clang warnings are as follows:
> ovpn-cli.c:1587:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>   1587 |         if (!sock) {
>        |             ^~~~~
> ovpn-cli.c:1635:9: note: uninitialized use occurs here
>   1635 |         return ret;
>        |                ^~~
> ovpn-cli.c:1587:2: note: remove the 'if' if its condition is always false
>   1587 |         if (!sock) {
>        |         ^~~~~~~~~~~~
>   1588 |                 fprintf(stderr, "cannot allocate netlink socket\n");
>        |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   1589 |                 goto err_free;
>        |                 ~~~~~~~~~~~~~~
>   1590 |         }
>        |         ~
> ovpn-cli.c:1584:15: note: initialize the variable 'ret' to silence this warning
>   1584 |         int mcid, ret;
>        |                      ^
>        |                       = 0
> ovpn-cli.c:2107:7: warning: variable 'ret' is used uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>   2107 |         case CMD_INVALID:
>        |              ^~~~~~~~~~~
> ovpn-cli.c:2111:9: note: uninitialized use occurs here
>   2111 |         return ret;
>        |                ^~~
> ovpn-cli.c:1939:12: note: initialize the variable 'ret' to silence this warning
>   1939 |         int n, ret;
>        |                   ^
>        |
> 
> 
> Fixes: 959bc330a439 ("testing/selftests: add test tool and scripts for ovpn module")
> ovpn module")
> Signed-off-by: Sidharth Seela <sidharthseela@...il.com>
> ---
> v4:
> 	- Move changelog below sign-off.
> 	- Remove double-hyphens in commit description.
> v3:
> 	- Use prefix net.
> 	- Remove so_txtime fix as default case calls error().
> 	- Changelog before sign-off.
> 	- Three dashes after sign-off
> v2:
> 	- Use subsystem name "net".
> 	- Add fixes tags.
> 	- Remove txtimestamp fix as default case calls error.
> 	- Assign constant error string instead of NULL.
> 
> 
> diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c
> index 9201f2905f2c..20d00378f34a 100644
> --- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
> +++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
> @@ -1581,7 +1581,7 @@ static int ovpn_listen_mcast(void)
>   {
>   	struct nl_sock *sock;
>   	struct nl_cb *cb;
> -	int mcid, ret;
> +	int mcid, ret = -1;

ret goes uninitialized only under the "if (!sock)" condition, therefore 
I'd rather assign ret a meaningful value instead of -1.

How about adding "err = -ENOMEM;" directly inside the if block?

>   
>   	sock = nl_socket_alloc();
>   	if (!sock) {
> @@ -1936,7 +1936,7 @@ static int ovpn_run_cmd(struct ovpn_ctx *ovpn)
>   {
>   	char peer_id[10], vpnip[INET6_ADDRSTRLEN], laddr[128], lport[10];
>   	char raddr[128], rport[10];
> -	int n, ret;
> +	int n, ret = -1;

Same here.
ret goes uninitialized only under the "CMD_INVALID" case.

How about adding "ret = -EINVAL;" inside the affected case?


Both values are returned by ovpn_run_cmd() and then printed as 
strerror(-ret).
If we blindly use -1 we will get "Operation not permitted" which will 
confuse the user IMHO.

Thanks a lot!

>   	FILE *fp;
>   
>   	switch (ovpn->cmd) {

-- 
Antonio Quartulli
OpenVPN Inc.