diff mbox series

[2/2] selftests/net: fix uninitialized variables

Message ID 20240505222639.70317-2-jhubbard@nvidia.com (mailing list archive)
State New
Headers show
Series [1/2] selftests/net: suppress clang's "variable-sized type not at the end" warning | expand

Commit Message

John Hubbard May 5, 2024, 10:26 p.m. UTC
When building with clang, via:

    make LLVM=1 -C tools/testing/selftest

...clang warns about three variables that are not initialized in all
cases:

1) The opt_ipproto_off variable is used uninitialized if "testname" is
not "ip". This seems like an actual bug.

2) The addr_len is used uninitialized, but only in the assert case,
   which bails out, so this is harmless.

3) The family variable in add_listener() is only used uninitialized in
   the error case (neither IPv4 nor IPv6 is specified), so it's also
   harmless.

Fix by initializing each variable.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/net/gro.c                 | 3 ++-
 tools/testing/selftests/net/ip_local_port_range.c | 2 +-
 tools/testing/selftests/net/mptcp/pm_nl_ctl.c     | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

Comments

Matthieu Baerts May 6, 2024, 7:49 a.m. UTC | #1
Hi John,

On 06/05/2024 00:26, John Hubbard wrote:
> When building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftest
> 
> ...clang warns about three variables that are not initialized in all
> cases:
> 
> 1) The opt_ipproto_off variable is used uninitialized if "testname" is
> not "ip". This seems like an actual bug.
> 
> 2) The addr_len is used uninitialized, but only in the assert case,
>    which bails out, so this is harmless.
> 
> 3) The family variable in add_listener() is only used uninitialized in
>    the error case (neither IPv4 nor IPv6 is specified), so it's also
>    harmless.
> 
> Fix by initializing each variable.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/net/gro.c                 | 3 ++-
>  tools/testing/selftests/net/ip_local_port_range.c | 2 +-
>  tools/testing/selftests/net/mptcp/pm_nl_ctl.c     | 2 +-

Thank you for fixing these warnings!

The modification in the MPTCP selftest directory looks good to me:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Cheers,
Matt
Willem de Bruijn May 6, 2024, 6 p.m. UTC | #2
John Hubbard wrote:
> When building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftest
> 
> ...clang warns about three variables that are not initialized in all
> cases:
> 
> 1) The opt_ipproto_off variable is used uninitialized if "testname" is
> not "ip". This seems like an actual bug.
> 
> 2) The addr_len is used uninitialized, but only in the assert case,
>    which bails out, so this is harmless.
> 
> 3) The family variable in add_listener() is only used uninitialized in
>    the error case (neither IPv4 nor IPv6 is specified), so it's also
>    harmless.
> 
> Fix by initializing each variable.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/net/gro.c                 | 3 ++-
>  tools/testing/selftests/net/ip_local_port_range.c | 2 +-
>  tools/testing/selftests/net/mptcp/pm_nl_ctl.c     | 2 +-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
> index 353e1e867fbb..0eb61edaad83 100644
> --- a/tools/testing/selftests/net/gro.c
> +++ b/tools/testing/selftests/net/gro.c
> @@ -110,7 +110,8 @@ static void setup_sock_filter(int fd)
>  	const int dport_off = tcp_offset + offsetof(struct tcphdr, dest);
>  	const int ethproto_off = offsetof(struct ethhdr, h_proto);
>  	int optlen = 0;
> -	int ipproto_off, opt_ipproto_off;
> +	int ipproto_off;
> +	int opt_ipproto_off = 0;

This is only intended to be used in the case where the IP proto is not TCP:

                        BPF_STMT(BPF_LD  + BPF_B   + BPF_ABS, ipproto_off),
+                       BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 2, 0),
+                       BPF_STMT(BPF_LD  + BPF_B   + BPF_ABS, opt_ipproto_off),
                        BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 0, 5),

In that case the test tries again at a different offset that accounts
for optional IPv6 extension headers.

This is indeed buggy, in that it might accidentally accept packets
that should be dropped.

Initializing to 0 compares against against the first byte of the
Ethernet header. Which is an external argument to the test. So
safest is to initialize opt_ipproto_off to ipproto_off and just
repeat the previous check. Perhaps:

@@ -118,6 +118,7 @@ static void setup_sock_filter(int fd)
        else
                next_off = offsetof(struct ipv6hdr, nexthdr);
        ipproto_off = ETH_HLEN + next_off;
+       opt_ipproto_off = ipproto_off;  /* overridden later if may have exthdrs */
John Hubbard May 6, 2024, 6:50 p.m. UTC | #3
On 5/6/24 11:00 AM, Willem de Bruijn wrote:
> John Hubbard wrote:
...
>> diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
>> index 353e1e867fbb..0eb61edaad83 100644
>> --- a/tools/testing/selftests/net/gro.c
>> +++ b/tools/testing/selftests/net/gro.c
>> @@ -110,7 +110,8 @@ static void setup_sock_filter(int fd)
>>   	const int dport_off = tcp_offset + offsetof(struct tcphdr, dest);
>>   	const int ethproto_off = offsetof(struct ethhdr, h_proto);
>>   	int optlen = 0;
>> -	int ipproto_off, opt_ipproto_off;
>> +	int ipproto_off;
>> +	int opt_ipproto_off = 0;
> 
> This is only intended to be used in the case where the IP proto is not TCP:
> 
>                          BPF_STMT(BPF_LD  + BPF_B   + BPF_ABS, ipproto_off),
> +                       BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 2, 0),
> +                       BPF_STMT(BPF_LD  + BPF_B   + BPF_ABS, opt_ipproto_off),
>                          BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_TCP, 0, 5),
> 
> In that case the test tries again at a different offset that accounts
> for optional IPv6 extension headers.
> 
> This is indeed buggy, in that it might accidentally accept packets
> that should be dropped.
> 
> Initializing to 0 compares against against the first byte of the
> Ethernet header. Which is an external argument to the test. So
> safest is to initialize opt_ipproto_off to ipproto_off and just
> repeat the previous check. Perhaps:
> 
> @@ -118,6 +118,7 @@ static void setup_sock_filter(int fd)
>          else
>                  next_off = offsetof(struct ipv6hdr, nexthdr);
>          ipproto_off = ETH_HLEN + next_off;
> +       opt_ipproto_off = ipproto_off;  /* overridden later if may have exthdrs */

OK, thanks for pointing out the right fix, I'll send a v2 that does that.

thanks,
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
index 353e1e867fbb..0eb61edaad83 100644
--- a/tools/testing/selftests/net/gro.c
+++ b/tools/testing/selftests/net/gro.c
@@ -110,7 +110,8 @@  static void setup_sock_filter(int fd)
 	const int dport_off = tcp_offset + offsetof(struct tcphdr, dest);
 	const int ethproto_off = offsetof(struct ethhdr, h_proto);
 	int optlen = 0;
-	int ipproto_off, opt_ipproto_off;
+	int ipproto_off;
+	int opt_ipproto_off = 0;
 	int next_off;
 
 	if (proto == PF_INET)
diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
index 193b82745fd8..29451d2244b7 100644
--- a/tools/testing/selftests/net/ip_local_port_range.c
+++ b/tools/testing/selftests/net/ip_local_port_range.c
@@ -359,7 +359,7 @@  TEST_F(ip_local_port_range, late_bind)
 		struct sockaddr_in v4;
 		struct sockaddr_in6 v6;
 	} addr;
-	socklen_t addr_len;
+	socklen_t addr_len = 0;
 	const int one = 1;
 	int fd, err;
 	__u32 range;
diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
index 7426a2cbd4a0..7ad5a59adff2 100644
--- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
+++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
@@ -1276,7 +1276,7 @@  int add_listener(int argc, char *argv[])
 	struct sockaddr_storage addr;
 	struct sockaddr_in6 *a6;
 	struct sockaddr_in *a4;
-	u_int16_t family;
+	u_int16_t family = AF_UNSPEC;
 	int enable = 1;
 	int sock;
 	int err;