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

Message ID 20150324211541.GA2318@blokker.redhat.com
State Committed
Headers

Commit Message

Mark Wielaard March 24, 2015, 9:15 p.m. UTC
  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.
  

Comments

Roland McGrath March 25, 2015, 9:25 p.m. UTC | #1
Please participate in the other thread trying to figure out a testing
strategy.  I think it's tractable.
  
Mark Wielaard March 26, 2015, 8:28 a.m. UTC | #2
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
  

Patch

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.  */