[patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[DJ Delorie]

https://sourceware.org/bugzilla/show_bug.cgi?id=22489

The patch in the bz applies and builds without change.  I added my
analysis of the failure (newer gcc's give a much more useful message
;) although it's pretty obvious in retrospect.

Only four lines changed, well under the assignment threshold.

From: Sergey <s.korolev@ndmsystems.com>
Date: Fri, 24 Apr 2020 17:18:41 -0400
Subject: Use unsigned constants for ICMP6 filters [BZ #22489]

The core problem here is that the bitfields are unsigned but
the computed constants are signed.  This patch forces the computed
constants to be unsigned. - DJ

diff --git a/inet/netinet/icmp6.h b/inet/netinet/icmp6.h
index a75722887d..5fed0fbca1 100644
--- a/inet/netinet/icmp6.h
+++ b/inet/netinet/icmp6.h
@@ -85,16 +85,16 @@ struct icmp6_hdr
 #define ICMP6_PARAMPROB_OPTION        2 /* unrecognized IPv6 option */
 
 #define ICMP6_FILTER_WILLPASS(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) & (1 << ((type) & 31))) == 0)
+	((((filterp)->icmp6_filt[(type) >> 5]) & (1U << ((type) & 31))) == 0)
 
 #define ICMP6_FILTER_WILLBLOCK(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) & (1 << ((type) & 31))) != 0)
+	((((filterp)->icmp6_filt[(type) >> 5]) & (1U << ((type) & 31))) != 0)
 
 #define ICMP6_FILTER_SETPASS(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1 << ((type) & 31))))
+	((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1U << ((type) & 31))))
 
 #define ICMP6_FILTER_SETBLOCK(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) |=  (1 << ((type) & 31))))
+	((((filterp)->icmp6_filt[(type) >> 5]) |=  (1U << ((type) & 31))))
 
 #define ICMP6_FILTER_SETPASSALL(filterp) \
 	memset (filterp, 0, sizeof (struct icmp6_filter));

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Andreas Schwab]

On Apr 24 2020, DJ Delorie via Libc-alpha wrote:

> The core problem here is that the bitfields are unsigned but
> the computed constants are signed.  This patch forces the computed
> constants to be unsigned. - DJ

That explanation doesn't make sense.  This has nothing to do with
signedness of bitfields, it is about shifting a value out of its range.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[DJ Delorie]

Andreas Schwab <schwab@linux-m68k.org> writes:
> That explanation doesn't make sense.  This has nothing to do with
> signedness of bitfields, it is about shifting a value out of its range.

The struct has this:

    uint32_t icmp6_filt[8];

It's not a "gcc bitfield" it's an "icmp filter bitfield"

The logic in the test case boils down to:

  (uint32_t) icmp6_filt[]  &=  (int) -65

GCC complains that -65 doesn't fit in "uint32_t" and was converted to
(unsigned) 0xffffffbf.

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Andreas Schwab]

On Apr 25 2020, DJ Delorie wrote:

> GCC complains that -65 doesn't fit in "uint32_t" and was converted to
> (unsigned) 0xffffffbf.

-65 fits very well in uint32_t, this is a perfectly defined operation.
The bug is an undefined shift (which can produce an arbitrary value).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[DJ Delorie]

Andreas Schwab <schwab@linux-m68k.org> writes:
>> GCC complains that -65 doesn't fit in "uint32_t" and was converted to
>> (unsigned) 0xffffffbf.
>
> -65 fits very well in uint32_t, this is a perfectly defined operation.
> The bug is an undefined shift (which can produce an arbitrary value).

The left shift is limited to "1" shifted by 0..31 bits, which is well
defined.  A shift of 31 bits doesn't result in a warning, either,
because the resulting ~bit pattern indicates a positive value.  The
right shift might produce an array overflow, but that would happen even
for unsigned values, and the warning happens for constants that are
known to fit.  GCC indicates the warning is with the &= operator:

$ gcc -c -Wsign-conversion icmp6_test.c
icmp6_test.c: In function ‘main’:
icmp6_test.c:14:7: warning: unsigned conversion from ‘int’ to ‘uint32_t’ {aka ‘unsigned int’} changes value from ‘-65’ to ‘4294967231’ [-Wsign-conversion]
   14 |       &= ~(1 <<
      |       ^~


#include <stdlib.h>                                                           
#include <netinet/icmp6.h>                                                    

#undef ICMP6_FILTER_SETPASS

#define ICMP6_FILTER_SETPASS(type, filterp) \
  ((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1 << ((type) & 31))))
                                                                           
int main()                                                                    
{                                                                             
    struct icmp6_filter filter;                                               
                                                                              
    (((filter.icmp6_filt[(134) >> 5])
      &= ~(1 <<
	   ((134) & 31))));
                                                                              
    return EXIT_SUCCESS;                                                      
}

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Andreas Schwab]

On Apr 25 2020, DJ Delorie wrote:

> The left shift is limited to "1" shifted by 0..31 bits, which is well
> defined.

Nope.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[DJ Delorie]

Andreas Schwab <schwab@linux-m68k.org> writes:
> Nope.

Well, feel free to offer an alternate comment for the patch - preferably
something more convincing than "nope".  Do you have any argument against
the patch itself?

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Florian Weimer]

* DJ Delorie via Libc-alpha:

> From: Sergey <s.korolev@ndmsystems.com>
> Date: Fri, 24 Apr 2020 17:18:41 -0400
> Subject: Use unsigned constants for ICMP6 filters [BZ #22489]
>
> The core problem here is that the bitfields are unsigned but
> the computed constants are signed.  This patch forces the computed
> constants to be unsigned. - DJ

I suggest to write “the icmp6_filt array elements are unsigned”
instead of “the bitfields are unsigned”.

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Alexander Monakov]

Computing 1<<31 causes undefined behavior (signed overflow), you get
a warning with -Wshift-overflow=2 and a runtime error with UBSan,
i.e. with -fsanitize=undefined.

I'd say the situation with -Wsign-conversion warning is more subtle
than your initial mail seemed to imply, normally gcc suppresses warnings
for code originating in system headers unless -Wsystem-headers is also
specified. Here the warning appears where user code expands a macro
defined in a system header, maybe this situation is not properly
handled for -Wsign-conversion in GCC.

In summary, you need 1u to avoid causing undefined behavior if
a shift left by 31 is possible in practice, and it also happens to
suppress GCC's -Wsign-conversion warning (while there's
nothing undefined about the conversion itself).

Alexander

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Florian Weimer]

* Alexander Monakov via Libc-alpha:

> Computing 1<<31 causes undefined behavior (signed overflow), you get
> a warning with -Wshift-overflow=2 and a runtime error with UBSan,
> i.e. with -fsanitize=undefined.
>
> I'd say the situation with -Wsign-conversion warning is more subtle
> than your initial mail seemed to imply, normally gcc suppresses warnings
> for code originating in system headers unless -Wsystem-headers is also
> specified. Here the warning appears where user code expands a macro
> defined in a system header, maybe this situation is not properly
> handled for -Wsign-conversion in GCC.
>
> In summary, you need 1u to avoid causing undefined behavior if
> a shift left by 31 is possible in practice, and it also happens to
> suppress GCC's -Wsign-conversion warning (while there's
> nothing undefined about the conversion itself).

I believe not of the bits in the array have index 31 at present, so
the undefined behavior cannot really occur in practice at this point.
Perhaps it makes sense to mention both aspects in the commit message,
though.

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[DJ Delorie]

How's this?

From aa8ad2eacb64a5e0e81892b87665fa50f66ad87a Mon Sep 17 00:00:00 2001
From: Sergey <s.korolev@ndmsystems.com>
Date: Fri, 24 Apr 2020 17:18:41 -0400
Subject: Use unsigned constants for ICMP6 filters [BZ #22489]

The core problem here is that the filter array is unsigned but the
computed constants are signed.  This both causes a signededness
conversion at the &= step and may cause undefined behavior if the MSB
is being modified.  This patch uses unsigned constants to avoid both
cases. - DJ

diff --git a/inet/netinet/icmp6.h b/inet/netinet/icmp6.h
index a75722887d..5fed0fbca1 100644
--- a/inet/netinet/icmp6.h
+++ b/inet/netinet/icmp6.h
@@ -85,16 +85,16 @@ struct icmp6_hdr
 #define ICMP6_PARAMPROB_OPTION        2 /* unrecognized IPv6 option */
 
 #define ICMP6_FILTER_WILLPASS(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) & (1 << ((type) & 31))) == 0)
+	((((filterp)->icmp6_filt[(type) >> 5]) & (1U << ((type) & 31))) == 0)
 
 #define ICMP6_FILTER_WILLBLOCK(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) & (1 << ((type) & 31))) != 0)
+	((((filterp)->icmp6_filt[(type) >> 5]) & (1U << ((type) & 31))) != 0)
 
 #define ICMP6_FILTER_SETPASS(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1 << ((type) & 31))))
+	((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1U << ((type) & 31))))
 
 #define ICMP6_FILTER_SETBLOCK(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) |=  (1 << ((type) & 31))))
+	((((filterp)->icmp6_filt[(type) >> 5]) |=  (1U << ((type) & 31))))
 
 #define ICMP6_FILTER_SETPASSALL(filterp) \
 	memset (filterp, 0, sizeof (struct icmp6_filter));

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Florian Weimer]

* DJ Delorie:

> The core problem here is that the filter array is unsigned but the
> computed constants are signed.  This both causes a signededness
> conversion at the &= step and may cause undefined behavior if the MSB
> is being modified.  This patch uses unsigned constants to avoid both
> cases. - DJ

I'd say that the filter array *elements* are unsigned, but I don't
feel strongly about that.

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[DJ Delorie]

Florian Weimer <fw@deneb.enyo.de> writes:
> I'd say that the filter array *elements* are unsigned, but I don't
> feel strongly about that.

Ok, hopefully one last version and then we can paint the bike shed ;-)

From 4ce3470246e0336e53010d66f30a1040a7e1f4bb Mon Sep 17 00:00:00 2001
From: Sergey <s.korolev@ndmsystems.com>
Date: Fri, 24 Apr 2020 17:18:41 -0400
Subject: Use unsigned constants for ICMP6 filters [BZ #22489]

The core problem here is that the filter array elements are unsigned
but the computed constants are signed.  This both causes a
signededness conversion at the &= step and may cause undefined
behavior if the MSB is being modified.  This patch uses unsigned
constants to avoid both cases. - DJ

diff --git a/inet/netinet/icmp6.h b/inet/netinet/icmp6.h
index a75722887d..5fed0fbca1 100644
--- a/inet/netinet/icmp6.h
+++ b/inet/netinet/icmp6.h
@@ -85,16 +85,16 @@ struct icmp6_hdr
 #define ICMP6_PARAMPROB_OPTION        2 /* unrecognized IPv6 option */
 
 #define ICMP6_FILTER_WILLPASS(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) & (1 << ((type) & 31))) == 0)
+	((((filterp)->icmp6_filt[(type) >> 5]) & (1U << ((type) & 31))) == 0)
 
 #define ICMP6_FILTER_WILLBLOCK(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) & (1 << ((type) & 31))) != 0)
+	((((filterp)->icmp6_filt[(type) >> 5]) & (1U << ((type) & 31))) != 0)
 
 #define ICMP6_FILTER_SETPASS(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1 << ((type) & 31))))
+	((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1U << ((type) & 31))))
 
 #define ICMP6_FILTER_SETBLOCK(type, filterp) \
-	((((filterp)->icmp6_filt[(type) >> 5]) |=  (1 << ((type) & 31))))
+	((((filterp)->icmp6_filt[(type) >> 5]) |=  (1U << ((type) & 31))))
 
 #define ICMP6_FILTER_SETPASSALL(filterp) \
 	memset (filterp, 0, sizeof (struct icmp6_filter));

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Florian Weimer]

* DJ Delorie via Libc-alpha:

> Florian Weimer <fw@deneb.enyo.de> writes:
>> I'd say that the filter array *elements* are unsigned, but I don't
>> feel strongly about that.
>
> Ok, hopefully one last version and then we can paint the bike shed ;-)
>
> From 4ce3470246e0336e53010d66f30a1040a7e1f4bb Mon Sep 17 00:00:00 2001
> From: Sergey <s.korolev@ndmsystems.com>
> Date: Fri, 24 Apr 2020 17:18:41 -0400
> Subject: Use unsigned constants for ICMP6 filters [BZ #22489]
>
> The core problem here is that the filter array elements are unsigned
> but the computed constants are signed.  This both causes a
> signededness conversion at the &= step and may cause undefined
> behavior if the MSB is being modified.  This patch uses unsigned
> constants to avoid both cases. - DJ
>
> diff --git a/inet/netinet/icmp6.h b/inet/netinet/icmp6.h
> index a75722887d..5fed0fbca1 100644
> --- a/inet/netinet/icmp6.h
> +++ b/inet/netinet/icmp6.h
> @@ -85,16 +85,16 @@ struct icmp6_hdr
>  #define ICMP6_PARAMPROB_OPTION        2 /* unrecognized IPv6 option */
>  
>  #define ICMP6_FILTER_WILLPASS(type, filterp) \
> -	((((filterp)->icmp6_filt[(type) >> 5]) & (1 << ((type) & 31))) == 0)
> +	((((filterp)->icmp6_filt[(type) >> 5]) & (1U << ((type) & 31))) == 0)
>  
>  #define ICMP6_FILTER_WILLBLOCK(type, filterp) \
> -	((((filterp)->icmp6_filt[(type) >> 5]) & (1 << ((type) & 31))) != 0)
> +	((((filterp)->icmp6_filt[(type) >> 5]) & (1U << ((type) & 31))) != 0)
>  
>  #define ICMP6_FILTER_SETPASS(type, filterp) \
> -	((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1 << ((type) & 31))))
> +	((((filterp)->icmp6_filt[(type) >> 5]) &= ~(1U << ((type) & 31))))
>  
>  #define ICMP6_FILTER_SETBLOCK(type, filterp) \
> -	((((filterp)->icmp6_filt[(type) >> 5]) |=  (1 << ((type) & 31))))
> +	((((filterp)->icmp6_filt[(type) >> 5]) |=  (1U << ((type) & 31))))
>  
>  #define ICMP6_FILTER_SETPASSALL(filterp) \
>  	memset (filterp, 0, sizeof (struct icmp6_filter));

Looks good to me know.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> Looks good to me know.
>
> Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks, pushed!

Re: [patch] Use unsigned constants for ICMP6 filters [BZ #22489]

[Andreas Schwab]

On Mai 08 2020, DJ Delorie wrote:

> The core problem here is that the filter array elements are unsigned
> but the computed constants are signed.

This is of course bogus.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."