[PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mark Wielaard]

Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
will trigger undefined behaviour because it is defined as a 31 bit shift
against an signed integer. Fix by explicitly using an unsigned int.
---
 ChangeLog | 4 ++++
 elf/elf.h | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index b0a17b0..bdf899f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2015-03-24  Mark Wielaard  <mjw@redhat.com>
+
+	* elf/elf.h (SHF_EXCLUDE): Use unsigned 1 for shift.
+
 2015-03-23  Roland McGrath  <roland@hack.frob.com>
 
 	* libio/iofdopen.c: Move FD_FLAGS declaration into its first use,
diff --git a/elf/elf.h b/elf/elf.h
index 496f08d..960a3c3 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -371,7 +371,7 @@ typedef struct
 #define SHF_MASKPROC	     0xf0000000	/* Processor-specific */
 #define SHF_ORDERED	     (1 << 30)	/* Special ordering requirement
 					   (Solaris).  */
-#define SHF_EXCLUDE	     (1 << 31)	/* Section is excluded unless
+#define SHF_EXCLUDE	     (1U << 31)	/* Section is excluded unless
 					   referenced or allocated (Solaris).*/
 
 /* Section group handling.  */
-- 
1.8.3.1

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Szabolcs Nagy]

On 24/03/15 10:39, Mark Wielaard wrote:
> Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
> will trigger undefined behaviour because it is defined as a 31 bit shift
> against an signed integer. Fix by explicitly using an unsigned int.

there is another proposed patch for this

https://sourceware.org/ml/libc-alpha/2015-03/msg00287.html

>  ChangeLog | 4 ++++
>  elf/elf.h | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 

i think changelog entries are supposed to be submitted separately

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On 24 Mar 2015 14:13, Szabolcs Nagy wrote:
> On 24/03/15 10:39, Mark Wielaard wrote:
> > Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
> > will trigger undefined behaviour because it is defined as a 31 bit shift
> > against an signed integer. Fix by explicitly using an unsigned int.
> 
> there is another proposed patch for this
> 
> https://sourceware.org/ml/libc-alpha/2015-03/msg00287.html
> 
> >  ChangeLog | 4 ++++
> >  elf/elf.h | 2 +-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> 
> i think changelog entries are supposed to be submitted separately

nope ... should be same commit
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Florian Weimer]

* Mike Frysinger:

> On 24 Mar 2015 14:13, Szabolcs Nagy wrote:
>> On 24/03/15 10:39, Mark Wielaard wrote:
>> > Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
>> > will trigger undefined behaviour because it is defined as a 31 bit shift
>> > against an signed integer. Fix by explicitly using an unsigned int.
>> 
>> there is another proposed patch for this
>> 
>> https://sourceware.org/ml/libc-alpha/2015-03/msg00287.html
>> 
>> >  ChangeLog | 4 ++++
>> >  elf/elf.h | 2 +-
>> >  2 files changed, 5 insertions(+), 1 deletion(-)
>> > 
>> 
>> i think changelog entries are supposed to be submitted separately
>
> nope ... should be same commit

There used be an expectation that the ChangeLog update was not to be
submitted as part of the patch, but as a separate item in the patch
submission (not as a diff hunk, not with + prefix).

Has this changed?  It would certainly make things simpler for me. :)

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

On Tue, Mar 24, 2015 at 02:13:44PM +0000, Szabolcs Nagy wrote:
> 
> On 24/03/15 10:39, Mark Wielaard wrote:
> > Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
> > will trigger undefined behaviour because it is defined as a 31 bit shift
> > against an signed integer. Fix by explicitly using an unsigned int.
> 
> there is another proposed patch for this
> 
> https://sourceware.org/ml/libc-alpha/2015-03/msg00287.html

I missed that one. It does seem more ambitious than what I am proposing.
It is probably a good idea to change every constant to the appropriate
unsigned type. But the testing requirements seem hard to satisfy and it
looks like that patch is stalled because of that.

Could this simpler patch that just fixes the one constant that does
have a real problem in practice when used be fixed independently?
I like building my project with gcc -fsanitize=undefined and the
usage of SHF_EXCLUDE is preventing that atm.

> >  ChangeLog | 4 ++++
> >  elf/elf.h | 2 +-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> 
> i think changelog entries are supposed to be submitted separately

OK. How about the following then?


I would appreciate it if someone could push that for me since I don't
have glibc commit access.

Thanks,

Mark

2015-03-24  Mark Wielaard  <mjw@redhat.com>

       * elf/elf.h (SHF_EXCLUDE): Use unsigned 1 for shift.

[-- Attachment #2: shf_exclude.patch --]
[-- Type: text/plain, Size: 500 bytes --]

diff --git a/elf/elf.h b/elf/elf.h
index 496f08d..960a3c3 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -371,7 +371,7 @@ typedef struct
 #define SHF_MASKPROC	     0xf0000000	/* Processor-specific */
 #define SHF_ORDERED	     (1 << 30)	/* Special ordering requirement
 					   (Solaris).  */
-#define SHF_EXCLUDE	     (1 << 31)	/* Section is excluded unless
+#define SHF_EXCLUDE	     (1U << 31)	/* Section is excluded unless
 					   referenced or allocated (Solaris).*/
 
 /* Section group handling.  */

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

On 24 Mar 2015 20:53, Florian Weimer wrote:
> * Mike Frysinger:
> > On 24 Mar 2015 14:13, Szabolcs Nagy wrote:
> >> On 24/03/15 10:39, Mark Wielaard wrote:
> >> > Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
> >> > will trigger undefined behaviour because it is defined as a 31 bit shift
> >> > against an signed integer. Fix by explicitly using an unsigned int.
> >> 
> >> there is another proposed patch for this
> >> 
> >> https://sourceware.org/ml/libc-alpha/2015-03/msg00287.html
> >> 
> >> >  ChangeLog | 4 ++++
> >> >  elf/elf.h | 2 +-
> >> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >> 
> >> i think changelog entries are supposed to be submitted separately
> >
> > nope ... should be same commit
> 
> There used be an expectation that the ChangeLog update was not to be
> submitted as part of the patch, but as a separate item in the patch
> submission (not as a diff hunk, not with + prefix).
> 
> Has this changed?  It would certainly make things simpler for me. :)

i don't know what the preference is, but i don't mind either
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Roland McGrath]

Please participate in the other thread trying to figure out a testing
strategy.  I think it's tractable.

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mark Wielaard]

On Wed, 2015-03-25 at 14:25 -0700, Roland McGrath wrote:
> Please participate in the other thread trying to figure out a testing
> strategy.  I think it's tractable.

To be honest I am not as optimistic and like my simple one character
patch better. But if others are as positive as you are then I certainly
don't mind a different solution. As long as the problem gets fixed.

Cheers,

Mark

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

Hi,

On Tue, 2015-03-24 at 22:15 +0100, Mark Wielaard wrote:
> On Tue, Mar 24, 2015 at 02:13:44PM +0000, Szabolcs Nagy wrote: 
> > On 24/03/15 10:39, Mark Wielaard wrote:
> > > Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
> > > will trigger undefined behaviour because it is defined as a 31 bit shift
> > > against an signed integer. Fix by explicitly using an unsigned int.
> > 
> > there is another proposed patch for this
> > 
> > https://sourceware.org/ml/libc-alpha/2015-03/msg00287.html
> 
> I missed that one. It does seem more ambitious than what I am proposing.
> It is probably a good idea to change every constant to the appropriate
> unsigned type. But the testing requirements seem hard to satisfy and it
> looks like that patch is stalled because of that.

So I participated in that other thread, but it seems a rewrite of elf.h
to use the "correct type" for each constant is not so tractable as
hoped. So it seems that patch is stalled. Since the SHF_EXCLUDE issue is
real (it keeps being reported over and over again against elfutils) and
easy to fix itself with this one character patch, could you reconsider
just applying this now?

> Could this simpler patch that just fixes the one constant that does
> have a real problem in practice when used be fixed independently?
> I like building my project with gcc -fsanitize=undefined and the
> usage of SHF_EXCLUDE is preventing that atm.
> 
> > >  ChangeLog | 4 ++++
> > >  elf/elf.h | 2 +-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > 
> > i think changelog entries are supposed to be submitted separately
> 
> OK. How about the following then?
>
> I would appreciate it if someone could push that for me since I don't
> have glibc commit access.

> 2015-03-24  Mark Wielaard  <mjw@redhat.com>
> 
>        * elf/elf.h (SHF_EXCLUDE): Use unsigned 1 for shift.


[-- Attachment #2: shf_exclude.patch --]
[-- Type: text/x-patch, Size: 513 bytes --]

diff --git a/elf/elf.h b/elf/elf.h
index 496f08d..960a3c3 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -371,7 +371,7 @@ typedef struct
 #define SHF_MASKPROC	     0xf0000000	/* Processor-specific */
 #define SHF_ORDERED	     (1 << 30)	/* Special ordering requirement
 					   (Solaris).  */
-#define SHF_EXCLUDE	     (1 << 31)	/* Section is excluded unless
+#define SHF_EXCLUDE	     (1U << 31)	/* Section is excluded unless
 					   referenced or allocated (Solaris).*/
 
 /* Section group handling.  */

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Florian Weimer]

On 04/21/2015 11:20 AM, Mark Wielaard wrote:
> -#define SHF_EXCLUDE	     (1 << 31)	/* Section is excluded unless
> +#define SHF_EXCLUDE	     (1U << 31)	/* Section is excluded unless

I think the safer change is to use -0x80000000 as the value of the
constant, without making it unsigned.  Otherwise my previous objections
apply.

I'm sorry it has come to this, I can empathize with your struggles to
get patches applied to glibc.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> I think the safer change is to use -0x80000000 as the value of the
> constant, without making it unsigned.

-0x80000000 has unsigned type.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Florian Weimer]

On 04/22/2015 10:25 AM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> I think the safer change is to use -0x80000000 as the value of the
>> constant, without making it unsigned.
> 
> -0x80000000 has unsigned type.

Ugh.  The joys of C.

Using a cast would rely on a GCC extension, so one has to write
(-2147483647 - 1) to get the desired value.  Wow.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mark Wielaard]

On Wed, 2015-04-22 at 10:33 +0200, Florian Weimer wrote:
> On 04/22/2015 10:25 AM, Andreas Schwab wrote:
> > Florian Weimer <fweimer@redhat.com> writes:
> > 
> >> I think the safer change is to use -0x80000000 as the value of the
> >> constant, without making it unsigned.
> > 
> > -0x80000000 has unsigned type.
> 
> Ugh.  The joys of C.
> 
> Using a cast would rely on a GCC extension, so one has to write
> (-2147483647 - 1) to get the desired value.  Wow.

What is your objection to having the constant unsigned?

Note that other ELF based systems like android or bsd just define the
constant as 0x80000000 (with or without UL). And the other SHF_
constants are also defined with their 0x... values in gABI. They are
flag values you combine with | or mask with &, not compare directly.

If you objection is that it isn't consistent with the other flag value
defines then my other proposal was to just define all these SHF
constants with their hex value:
https://sourceware.org/ml/libc-alpha/2015-03/msg00793.html

Cheers,

Mark

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Alexander Cherepanov]

On 2015-04-22 11:33, Florian Weimer wrote:
> On 04/22/2015 10:25 AM, Andreas Schwab wrote:
>> Florian Weimer <fweimer@redhat.com> writes:
>>
>>> I think the safer change is to use -0x80000000 as the value of the
>>> constant, without making it unsigned.
>>
>> -0x80000000 has unsigned type.
>
> Ugh.  The joys of C.
>
> Using a cast would rely on a GCC extension, so one has to write
> (-2147483647 - 1) to get the desired value.  Wow.

Given that stdint.h is already included it's perhaps easier to just use 
INT32_MIN?

I've replied about it and other issues in the previous thread but I'm 
afraid I'm lost all Cc there. Here it is:

https://sourceware.org/ml/libc-alpha/2015-04/msg00257.html

-- 
Alexander Cherepanov

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Florian Weimer]

On 04/22/2015 10:14 AM, Florian Weimer wrote:
> On 04/21/2015 11:20 AM, Mark Wielaard wrote:
>> -#define SHF_EXCLUDE	     (1 << 31)	/* Section is excluded unless
>> +#define SHF_EXCLUDE	     (1U << 31)	/* Section is excluded unless
> 
> I think the safer change is to use -0x80000000 as the value of the
> constant, without making it unsigned.  Otherwise my previous objections
> apply.

I thought some more about this, and have changed my opinion completely.
 Making the constant unsigned is less risky than making it negative
because of potential sign extension issues.  It's the lesser of two evils.

The proposed patch is okay with me.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

On Wed, 2015-04-22 at 11:18 +0200, Florian Weimer wrote:
> On 04/22/2015 10:14 AM, Florian Weimer wrote:
> > On 04/21/2015 11:20 AM, Mark Wielaard wrote:
> >> -#define SHF_EXCLUDE	     (1 << 31)	/* Section is excluded unless
> >> +#define SHF_EXCLUDE	     (1U << 31)	/* Section is excluded unless
> > 
> > I think the safer change is to use -0x80000000 as the value of the
> > constant, without making it unsigned.  Otherwise my previous objections
> > apply.
> 
> I thought some more about this, and have changed my opinion completely.
>  Making the constant unsigned is less risky than making it negative
> because of potential sign extension issues.  It's the lesser of two evils.
> 
> The proposed patch is okay with me.

Thanks. I didn't see other objections. So if it is good to go in could
someone please push it for me? (I don't have glibc git push access.)

ChangeLog

       * elf/elf.h (SHF_EXCLUDE): Use unsigned 1 for shift.


[-- Attachment #2: shf_exclude.patch --]
[-- Type: text/x-patch, Size: 1084 bytes --]

From 86771e8963653c306e53c07e1640914081afb30a Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Tue, 24 Mar 2015 11:32:36 +0100
Subject: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined
 behaviour.

Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
will trigger undefined behaviour because it is defined as a 31 bit shift
against an signed integer. Fix by explicitly using an unsigned int.
---
 ChangeLog | 4 ++++
 elf/elf.h | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/elf/elf.h b/elf/elf.h
index 71492a2..39bafc2 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -371,7 +371,7 @@ typedef struct
 #define SHF_MASKPROC	     0xf0000000	/* Processor-specific */
 #define SHF_ORDERED	     (1 << 30)	/* Special ordering requirement
 					   (Solaris).  */
-#define SHF_EXCLUDE	     (1 << 31)	/* Section is excluded unless
+#define SHF_EXCLUDE	     (1U << 31)	/* Section is excluded unless
 					   referenced or allocated (Solaris).*/
 
 /* Section group handling.  */
-- 
1.8.3.1

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Florian Weimer]

On 04/28/2015 10:22 AM, Mark Wielaard wrote:

> Thanks. I didn't see other objections. So if it is good to go in could
> someone please push it for me? (I don't have glibc git push access.)
> 
> ChangeLog
> 
>        * elf/elf.h (SHF_EXCLUDE): Use unsigned 1 for shift.
> 

Thanks, committed.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Ondřej Bílka]

On Tue, Mar 24, 2015 at 11:39:39AM +0100, Mark Wielaard wrote:
> Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
> will trigger undefined behaviour because it is defined as a 31 bit shift
> against an signed integer. Fix by explicitly using an unsigned int.
> ---
>  ChangeLog | 4 ++++
>  elf/elf.h | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
I looked that Florian acked this at patchwork thread. Florian, could you also
commit it or do we need more discussion?

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Mark Wielaard]

On Wed, 2015-05-13 at 12:27 +0200, Ondřej Bílka wrote:
> On Tue, Mar 24, 2015 at 11:39:39AM +0100, Mark Wielaard wrote:
> > Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
> > will trigger undefined behaviour because it is defined as a 31 bit shift
> > against an signed integer. Fix by explicitly using an unsigned int.
> > ---
> >  ChangeLog | 4 ++++
> >  elf/elf.h | 2 +-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> I looked that Florian acked this at patchwork thread. Florian, could you also
> commit it or do we need more discussion?

It is already in as commit fb4041, see:
https://sourceware.org/ml/libc-alpha/2015-04/msg00359.html

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Ondřej Bílka]

On Wed, May 13, 2015 at 12:46:26PM +0200, Mark Wielaard wrote:
> On Wed, 2015-05-13 at 12:27 +0200, Ondřej Bílka wrote:
> > On Tue, Mar 24, 2015 at 11:39:39AM +0100, Mark Wielaard wrote:
> > > Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
> > > will trigger undefined behaviour because it is defined as a 31 bit shift
> > > against an signed integer. Fix by explicitly using an unsigned int.
> > > ---
> > >  ChangeLog | 4 ++++
> > >  elf/elf.h | 2 +-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > I looked that Florian acked this at patchwork thread. Florian, could you also
> > commit it or do we need more discussion?
> 
> It is already in as commit fb4041, see:
> https://sourceware.org/ml/libc-alpha/2015-04/msg00359.html

thanks. marked that also at patchwork.

Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour.

[Carlos O'Donell]

On 05/13/2015 06:56 AM, Ondřej Bílka wrote:
> On Wed, May 13, 2015 at 12:46:26PM +0200, Mark Wielaard wrote:
>> On Wed, 2015-05-13 at 12:27 +0200, Ondřej Bílka wrote:
>>> On Tue, Mar 24, 2015 at 11:39:39AM +0100, Mark Wielaard wrote:
>>>> Any use of SHF_EXCLUDE in code that tries to check it against sh_flags
>>>> will trigger undefined behaviour because it is defined as a 31 bit shift
>>>> against an signed integer. Fix by explicitly using an unsigned int.
>>>> ---
>>>>  ChangeLog | 4 ++++
>>>>  elf/elf.h | 2 +-
>>>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>> I looked that Florian acked this at patchwork thread. Florian, could you also
>>> commit it or do we need more discussion?
>>
>> It is already in as commit fb4041, see:
>> https://sourceware.org/ml/libc-alpha/2015-04/msg00359.html
> 
> thanks. marked that also at patchwork.
> 

Ondrej,

Thanks for the patchwork cleanup!

c.