netdev - Re: [RFC] iproute2: ipila warning

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] [day] [month] [year] [list]

Date: Tue, 3 Oct 2023 11:35:47 +0200
From: Petr Machata <petrm@...dia.com>
To: Stephen Hemminger <stephen@...workplumber.org>
CC: <netdev@...r.kernel.org>
Subject: Re: [RFC] iproute2: ipila warning


Stephen Hemminger <stephen@...workplumber.org> writes:

> Building current code with Debian stable Gcc 12.2.0 see this warning.
>
>     CC       ipila.o
> ipila.c: In function ‘print_ila_locid’:
> ipila.c:57:32: warning: ‘addr’ may be used uninitialized [-Wmaybe-uninitialized]
>    57 |                 v = ntohs(words[i]);
>       |                                ^
> ipila.c:69:13: note: ‘addr’ declared here
>    69 | static void print_ila_locid(const char *tag, int attr, struct rtattr *tb[])
>       |             ^~~~~~~~~~~~~~~
>
> Looks like a Gcc aliasing bug.
> Relevant snippets.
>
> static void print_addr64(__u64 addr, char *buff, size_t len)
> {
> 	__u16 *words = (__u16 *)&addr;
> 	__u16 v;
> 	int i, ret;
> 	size_t written = 0;
> 	char *sep = ":";
>
> 	for (i = 0; i < 4; i++) {
> 		v = ntohs(words[i]);
> ...
>
>
> static void print_ila_locid(const char *tag, int attr, struct rtattr *tb[])
> {
> 	char abuf[256];
>
> 	if (tb[attr])
> 		print_addr64(rta_getattr_u64(tb[attr]),
> 			     abuf, sizeof(abuf));
>
> One solution would be to use a union.
> Other would be to use some variation of no-strict aliasing.
>
> --- a/ip/ipila.c
> +++ b/ip/ipila.c
> @@ -47,14 +47,17 @@ static int genl_family = -1;
>  
>  static void print_addr64(__u64 addr, char *buff, size_t len)
>  {
> -       __u16 *words = (__u16 *)&addr;
> +       union {
> +               __u64 w64;
> +               __u16 words[4];
> +       } id = { .w64 = addr };

This looks OK to me FWIW. Unions are commonly used to legalize aliasing,
so anybody looking at this will understand what's going on.

>         __u16 v;
>         int i, ret;
>         size_t written = 0;
>         char *sep = ":";
>  
>         for (i = 0; i < 4; i++) {
> -               v = ntohs(words[i]);
> +               v = ntohs(id.words[i]);
>  
>                 if (i == 3)
>                         sep = "";
> ..