| Message ID | ZpliTBli0quQfkke@mx3210.local |
|---|---|
| State | New |
| Headers | show |
| Series | Fix usage of _STACK_GROWS_DOWN and _STACK_GROWS_UP defines [BZ 31989] | expand |
John David Anglin <dave@mx3210.local> writes: (I think your From: is broken!) > When When glibc is configured with --enable-fortify-source, the > stdlib/tst-swapcontext2 test fails on hppa. The stack placement > for func1 and func2 are interchanged causing the test to fail. > > The attached patch fixes several places where the defines for > _STACK_GROWS_UP and_STACK_GROWS_DOWN are used. Both are defined > on all targets, so using #ifdef is incorrect. > > Tested on hppa-unknown-linux-gnu with no regressions. > > Okay for trunk? I know we are in the freeze for 2.40 release. Frustratingly, we've had this bug before elsewhere and fixed it in 2bbc694df279020a6620096d31c1e05c93966f9b (PR28899) The changes look right, checked git history for other errors, and started to compare binaries on amd64 out of paranoia given we're in freeze. I do wonder if we should tweak include/stackinfo.h as it has a sanity check but it checks the truthiness as well (not just whether it's defined). Not sure either way yet. Pending resolution of the question below for malloc/: Reviewed-by: Sam James <sam@gentoo.org> CC'd Andreas as RM. > > Dave > --- > > Fix usage of _STACK_GROWS_DOWN and _STACK_GROWS_UP defines [BZ 31989] > > diff --git a/malloc/memusage.c b/malloc/memusage.c > index e8ae80dc74..f80225b95a 100644 > --- a/malloc/memusage.c > +++ b/malloc/memusage.c > @@ -172,7 +172,7 @@ update_data (struct header *result, size_t len, size_t old_len) > start_sp = __thread_stack_pointer (); > > uintptr_t sp = __thread_stack_pointer (); > -#ifdef _STACK_GROWS_UP > +#if _STACK_GROWS_UP This change isn't HPPA-only, right? (because _STACK_GROWS_UP was defined everywhere, just to 0)? I wonder if we should defer this hunk until later. CC'ing DJ for advice on the malloc change. > /* This can happen in threads where we didn't catch the thread's > stack early enough. */ > if (__glibc_unlikely (sp < start_sp)) > diff --git a/stdlib/tst-swapcontext2.c b/stdlib/tst-swapcontext2.c > index f679755649..a9c1dc827c 100644 > --- a/stdlib/tst-swapcontext2.c > +++ b/stdlib/tst-swapcontext2.c > @@ -85,7 +85,7 @@ do_test (void) > { > /* ____longjmp_chk has */ > #if 0 > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN > #define called_from(this, saved) ((this) < (saved)) > #else > #define called_from(this, saved) ((this) > (saved)) > @@ -98,7 +98,7 @@ do_test (void) > /* Arrange stacks for uctx_func1 and uctx_func2 so that called_from > is true when setjmp is called from uctx_func1 and longjmp is called > from uctx_func2. */ > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN > # define UCTX_FUNC1_STACK 1 > # define UCTX_FUNC2_STACK 0 > #else > diff --git a/sysdeps/unix/sysv/linux/____longjmp_chk.c b/sysdeps/unix/sysv/linux/____longjmp_chk.c > index 0896dc5755..3c66a4638e 100644 > --- a/sysdeps/unix/sysv/linux/____longjmp_chk.c > +++ b/sysdeps/unix/sysv/linux/____longjmp_chk.c > @@ -23,7 +23,7 @@ > #include <stdio.h> > #include <stackinfo.h> > > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN > #define called_from(this, saved) ((this) < (saved)) > #else > #define called_from(this, saved) ((this) > (saved))
On 7/18/24 2:43 PM, John David Anglin wrote: > When When glibc is configured with --enable-fortify-source, the > stdlib/tst-swapcontext2 test fails on hppa. The stack placement > for func1 and func2 are interchanged causing the test to fail. > > The attached patch fixes several places where the defines for > _STACK_GROWS_UP and_STACK_GROWS_DOWN are used. Both are defined > on all targets, so using #ifdef is incorrect. > > Tested on hppa-unknown-linux-gnu with no regressions. > > Okay for trunk? I know we are in the freeze for 2.40 release. So Sam has reviewed this and DJ has commented. I'll note also that I think this is correct. There is some risk as a generic change to memsuage and the longjmp check but it's fairly minor. I think we can improve hppa for release with this change without too much other risk. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > Dave > --- > > Fix usage of _STACK_GROWS_DOWN and _STACK_GROWS_UP defines [BZ 31989] > > diff --git a/malloc/memusage.c b/malloc/memusage.c > index e8ae80dc74..f80225b95a 100644 > --- a/malloc/memusage.c > +++ b/malloc/memusage.c > @@ -172,7 +172,7 @@ update_data (struct header *result, size_t len, size_t old_len) > start_sp = __thread_stack_pointer (); > > uintptr_t sp = __thread_stack_pointer (); > -#ifdef _STACK_GROWS_UP > +#if _STACK_GROWS_UP OK. This is correct. It is generic code for memusage across all arches, but as DJ notes the stackinfo.h always defines these because that's how we want all Macro API to worksafely. > /* This can happen in threads where we didn't catch the thread's > stack early enough. */ > if (__glibc_unlikely (sp < start_sp)) > diff --git a/stdlib/tst-swapcontext2.c b/stdlib/tst-swapcontext2.c > index f679755649..a9c1dc827c 100644 > --- a/stdlib/tst-swapcontext2.c > +++ b/stdlib/tst-swapcontext2.c > @@ -85,7 +85,7 @@ do_test (void) > { > /* ____longjmp_chk has */ > #if 0 > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN OK. Also correct. Though in an #if 0 block. > #define called_from(this, saved) ((this) < (saved)) > #else > #define called_from(this, saved) ((this) > (saved)) > @@ -98,7 +98,7 @@ do_test (void) > /* Arrange stacks for uctx_func1 and uctx_func2 so that called_from > is true when setjmp is called from uctx_func1 and longjmp is called > from uctx_func2. */ > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN OK. Also correct. > # define UCTX_FUNC1_STACK 1 > # define UCTX_FUNC2_STACK 0 > #else > diff --git a/sysdeps/unix/sysv/linux/____longjmp_chk.c b/sysdeps/unix/sysv/linux/____longjmp_chk.c > index 0896dc5755..3c66a4638e 100644 > --- a/sysdeps/unix/sysv/linux/____longjmp_chk.c > +++ b/sysdeps/unix/sysv/linux/____longjmp_chk.c > @@ -23,7 +23,7 @@ > #include <stdio.h> > #include <stackinfo.h> > > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN OK. Correct. > #define called_from(this, saved) ((this) < (saved)) > #else > #define called_from(this, saved) ((this) > (saved))
Am Donnerstag, 18. Juli 2024, 20:43:24 CEST schrieb John David Anglin: > When When glibc is configured with --enable-fortify-source, the > stdlib/tst-swapcontext2 test fails on hppa. The stack placement > for func1 and func2 are interchanged causing the test to fail. > > The attached patch fixes several places where the defines for > _STACK_GROWS_UP and_STACK_GROWS_DOWN are used. Both are defined > on all targets, so using #ifdef is incorrect. > > Tested on hppa-unknown-linux-gnu with no regressions. > > Okay for trunk? I know we are in the freeze for 2.40 release. After collecting some opinions and poking around in the code myself- OK, please push it (with fixed e-mail). > > Dave > --- > > Fix usage of _STACK_GROWS_DOWN and _STACK_GROWS_UP defines [BZ 31989] > > diff --git a/malloc/memusage.c b/malloc/memusage.c > index e8ae80dc74..f80225b95a 100644 > --- a/malloc/memusage.c > +++ b/malloc/memusage.c > @@ -172,7 +172,7 @@ update_data (struct header *result, size_t len, size_t old_len) > start_sp = __thread_stack_pointer (); > > uintptr_t sp = __thread_stack_pointer (); > -#ifdef _STACK_GROWS_UP > +#if _STACK_GROWS_UP > /* This can happen in threads where we didn't catch the thread's > stack early enough. */ > if (__glibc_unlikely (sp < start_sp)) > diff --git a/stdlib/tst-swapcontext2.c b/stdlib/tst-swapcontext2.c > index f679755649..a9c1dc827c 100644 > --- a/stdlib/tst-swapcontext2.c > +++ b/stdlib/tst-swapcontext2.c > @@ -85,7 +85,7 @@ do_test (void) > { > /* ____longjmp_chk has */ > #if 0 > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN > #define called_from(this, saved) ((this) < (saved)) > #else > #define called_from(this, saved) ((this) > (saved)) > @@ -98,7 +98,7 @@ do_test (void) > /* Arrange stacks for uctx_func1 and uctx_func2 so that called_from > is true when setjmp is called from uctx_func1 and longjmp is called > from uctx_func2. */ > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN > # define UCTX_FUNC1_STACK 1 > # define UCTX_FUNC2_STACK 0 > #else > diff --git a/sysdeps/unix/sysv/linux/____longjmp_chk.c b/sysdeps/unix/sysv/linux/____longjmp_chk.c > index 0896dc5755..3c66a4638e 100644 > --- a/sysdeps/unix/sysv/linux/____longjmp_chk.c > +++ b/sysdeps/unix/sysv/linux/____longjmp_chk.c > @@ -23,7 +23,7 @@ > #include <stdio.h> > #include <stackinfo.h> > > -#ifdef _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN > #define called_from(this, saved) ((this) < (saved)) > #else > #define called_from(this, saved) ((this) > (saved)) >
diff --git a/malloc/memusage.c b/malloc/memusage.c index e8ae80dc74..f80225b95a 100644 --- a/malloc/memusage.c +++ b/malloc/memusage.c @@ -172,7 +172,7 @@ update_data (struct header *result, size_t len, size_t old_len) start_sp = __thread_stack_pointer (); uintptr_t sp = __thread_stack_pointer (); -#ifdef _STACK_GROWS_UP +#if _STACK_GROWS_UP /* This can happen in threads where we didn't catch the thread's stack early enough. */ if (__glibc_unlikely (sp < start_sp)) diff --git a/stdlib/tst-swapcontext2.c b/stdlib/tst-swapcontext2.c index f679755649..a9c1dc827c 100644 --- a/stdlib/tst-swapcontext2.c +++ b/stdlib/tst-swapcontext2.c @@ -85,7 +85,7 @@ do_test (void) { /* ____longjmp_chk has */ #if 0 -#ifdef _STACK_GROWS_DOWN +#if _STACK_GROWS_DOWN #define called_from(this, saved) ((this) < (saved)) #else #define called_from(this, saved) ((this) > (saved)) @@ -98,7 +98,7 @@ do_test (void) /* Arrange stacks for uctx_func1 and uctx_func2 so that called_from is true when setjmp is called from uctx_func1 and longjmp is called from uctx_func2. */ -#ifdef _STACK_GROWS_DOWN +#if _STACK_GROWS_DOWN # define UCTX_FUNC1_STACK 1 # define UCTX_FUNC2_STACK 0 #else diff --git a/sysdeps/unix/sysv/linux/____longjmp_chk.c b/sysdeps/unix/sysv/linux/____longjmp_chk.c index 0896dc5755..3c66a4638e 100644 --- a/sysdeps/unix/sysv/linux/____longjmp_chk.c +++ b/sysdeps/unix/sysv/linux/____longjmp_chk.c @@ -23,7 +23,7 @@ #include <stdio.h> #include <stackinfo.h> -#ifdef _STACK_GROWS_DOWN +#if _STACK_GROWS_DOWN #define called_from(this, saved) ((this) < (saved)) #else #define called_from(this, saved) ((this) > (saved))