Message ID | 1429608058.1938.53.camel@bordewijk.wildebeest.org |
---|---|
State | Committed |
Headers |
Received: (qmail 78777 invoked by alias); 21 Apr 2015 09:21:03 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 77624 invoked by uid 89); 21 Apr 2015 09:21:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL, BAYES_00, MIME_BASE64_BLANKS, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Message-ID: <1429608058.1938.53.camel@bordewijk.wildebeest.org> Subject: Re: [PATCH] elf.h SHF_EXCLUDE signed int 31 bit shift triggers undefined behaviour. From: Mark Wielaard <mjw@redhat.com> To: Szabolcs Nagy <szabolcs.nagy@arm.com> Cc: "libc-alpha@sourceware.org" <libc-alpha@sourceware.org>, Josh Stone <jistone@redhat.com> Date: Tue, 21 Apr 2015 11:20:58 +0200 In-Reply-To: <20150324211541.GA2318@blokker.redhat.com> References: <1427193579-26102-1-git-send-email-mjw@redhat.com> <55117118.1080706@arm.com> <20150324211541.GA2318@blokker.redhat.com> Content-Type: multipart/mixed; boundary="=-kRj1is6PfDaia5wNEMeY" Mime-Version: 1.0 |
Commit Message
Mark Wielaard
April 21, 2015, 9:20 a.m. UTC
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.
Comments
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 <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.
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.
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
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
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.
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. */