| Message ID | fa3615c4-6b12-27b1-9b53-2c5c1f40b87e@mentor.com |
|---|---|
| State | New |
| Headers | show |
On 09/15/2016 04:29 AM, Tom de Vries wrote: > On 31/08/16 07:42, Tom de Vries wrote: >> On 30/08/16 11:38, Bernd Edlinger wrote: >>> On 08/30/16 10:21, Tom de Vries wrote: >>>> On 29/08/16 18:43, Bernd Edlinger wrote: >>>>> Thanks! >>>>> >>>>> Actually my patch missed to fix one combination: -m32 with -fpic >>>>> >>>>> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c --tool_opts >>>>> '-m32 -fpic'" >>>>> >>>>> FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test >>>>> FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto >>>>> -fno-use-linker-plugin -flto-partition=none execution test >>>>> >>>>> The problem here is that the functions f2 and f3 access a stack- >>>>> based object out of bounds and that is inlined in main and >>>>> therefore smashes the return address of main in this case. >>>>> >>>>> A possible fix could look like follows: >>>>> >>>>> Index: object-size-9.c >>>>> =================================================================== >>>>> --- object-size-9.c (revision 239794) >>>>> +++ object-size-9.c (working copy) >>>>> @@ -93,5 +93,9 @@ >>>>> #endif >>>>> f4 (12); >>>>> f5 (12); >>>>> +#ifdef __cplusplus >>>>> + /* Stack may be smashed by f2/f3 above. */ >>>>> + __builtin_exit (0); >>>>> +#endif >>>>> return 0; >>>>> } >>>>> >>>>> >>>>> Do you think that this should be fixed too? >>>> >>>> I think it should be fixed. Ideally, we'd prevent the out-of-bounds >>>> writes to have harmful effects, but I'm not sure how to enforce that. >>>> >>>> This works for me: >>>> ... >>>> diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>> b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>> index 46f1fb9..fec920d 100644 >>>> --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>> +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>> @@ -31,6 +31,7 @@ static struct C >>>> f2 (int i) >>>> { >>>> struct C x; >>>> + struct C x2; >>>> x.d[i] = 'z'; >>>> return x; >>>> } >>>> @@ -45,6 +46,7 @@ static struct C >>>> f3 (int i) >>>> { >>>> struct C x; >>>> + struct C x2; >>>> char *p = x.d; >>>> p += i; >>>> *p = 'z'; >>>> ... >>>> >>>> But I have no idea how stable this solution is. >>>> >>> >>> At least x2 could not be opimized away, as it is no POD, >>> but there is no guarantee, that x2 comes after x on the stack. >>> Another possibility, which seems to work as well: >>> >>> >>> Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>> =================================================================== >>> --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c (revision >>> 239794) >>> +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c (working copy) >>> @@ -30,7 +30,7 @@ f1 (struct T x, int i) >>> static struct C >>> f2 (int i) >>> { >>> - struct C x; >>> + struct C x __attribute__ ((aligned(16))); >>> x.d[i] = 'z'; >>> return x; >>> } >>> @@ -44,7 +44,7 @@ f2 (int i) >>> static struct C >>> f3 (int i) >>> { >>> - struct C x; >>> + struct C x __attribute ((aligned(16))); >>> char *p = x.d; >>> p += i; >>> *p = 'z'; >>> >> >> Works for me. > > OK for trunk, 5 & 6 branch? > > Thanks, > - Tom > > > 0001-Fix-object-size-9.c-with-fpic.patch > > > Fix object-size-9.c with -fpic > > 2016-09-15 Bernd Edlinger <bernd.edlinger@hotmail.de> > Tom de Vries <tom@codesourcery.com> > > PR testsuite/77411 > * c-c++-common/ubsan/object-size-9.c (f2, f3): Declare struct C variable > with __attribute__((aligned(16))). Just so I'm sure on this stuff. The tests exist to verify that ubsan detects the out-of-bounds writes. ubsan isn't terminating the process, so we end up with a smashed stack? I fail to see how using aligned like this should consistently work. It feels like a hack that just happens to work now. Would it work to break this up into distinct tests, exit()-ing from each function rather than returning back to main? jeff
On 09/19/16 22:19, Jeff Law wrote: > On 09/15/2016 04:29 AM, Tom de Vries wrote: >> On 31/08/16 07:42, Tom de Vries wrote: >>> On 30/08/16 11:38, Bernd Edlinger wrote: >>>> On 08/30/16 10:21, Tom de Vries wrote: >>>>> On 29/08/16 18:43, Bernd Edlinger wrote: >>>>>> Thanks! >>>>>> >>>>>> Actually my patch missed to fix one combination: -m32 with -fpic >>>>>> >>>>>> make check-gcc-c++ RUNTESTFLAGS="ubsan.exp=object-size-9.c >>>>>> --tool_opts >>>>>> '-m32 -fpic'" >>>>>> >>>>>> FAIL: c-c++-common/ubsan/object-size-9.c -O2 execution test >>>>>> FAIL: c-c++-common/ubsan/object-size-9.c -O2 -flto >>>>>> -fno-use-linker-plugin -flto-partition=none execution test >>>>>> >>>>>> The problem here is that the functions f2 and f3 access a stack- >>>>>> based object out of bounds and that is inlined in main and >>>>>> therefore smashes the return address of main in this case. >>>>>> >>>>>> A possible fix could look like follows: >>>>>> >>>>>> Index: object-size-9.c >>>>>> =================================================================== >>>>>> --- object-size-9.c (revision 239794) >>>>>> +++ object-size-9.c (working copy) >>>>>> @@ -93,5 +93,9 @@ >>>>>> #endif >>>>>> f4 (12); >>>>>> f5 (12); >>>>>> +#ifdef __cplusplus >>>>>> + /* Stack may be smashed by f2/f3 above. */ >>>>>> + __builtin_exit (0); >>>>>> +#endif >>>>>> return 0; >>>>>> } >>>>>> >>>>>> >>>>>> Do you think that this should be fixed too? >>>>> >>>>> I think it should be fixed. Ideally, we'd prevent the out-of-bounds >>>>> writes to have harmful effects, but I'm not sure how to enforce that. >>>>> >>>>> This works for me: >>>>> ... >>>>> diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>>> b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>>> index 46f1fb9..fec920d 100644 >>>>> --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>>> +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>>> @@ -31,6 +31,7 @@ static struct C >>>>> f2 (int i) >>>>> { >>>>> struct C x; >>>>> + struct C x2; >>>>> x.d[i] = 'z'; >>>>> return x; >>>>> } >>>>> @@ -45,6 +46,7 @@ static struct C >>>>> f3 (int i) >>>>> { >>>>> struct C x; >>>>> + struct C x2; >>>>> char *p = x.d; >>>>> p += i; >>>>> *p = 'z'; >>>>> ... >>>>> >>>>> But I have no idea how stable this solution is. >>>>> >>>> >>>> At least x2 could not be opimized away, as it is no POD, >>>> but there is no guarantee, that x2 comes after x on the stack. >>>> Another possibility, which seems to work as well: >>>> >>>> >>>> Index: gcc/testsuite/c-c++-common/ubsan/object-size-9.c >>>> =================================================================== >>>> --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c (revision >>>> 239794) >>>> +++ gcc/testsuite/c-c++-common/ubsan/object-size-9.c (working copy) >>>> @@ -30,7 +30,7 @@ f1 (struct T x, int i) >>>> static struct C >>>> f2 (int i) >>>> { >>>> - struct C x; >>>> + struct C x __attribute__ ((aligned(16))); >>>> x.d[i] = 'z'; >>>> return x; >>>> } >>>> @@ -44,7 +44,7 @@ f2 (int i) >>>> static struct C >>>> f3 (int i) >>>> { >>>> - struct C x; >>>> + struct C x __attribute ((aligned(16))); >>>> char *p = x.d; >>>> p += i; >>>> *p = 'z'; >>>> >>> >>> Works for me. >> >> OK for trunk, 5 & 6 branch? >> >> Thanks, >> - Tom >> >> >> 0001-Fix-object-size-9.c-with-fpic.patch >> >> >> Fix object-size-9.c with -fpic >> >> 2016-09-15 Bernd Edlinger <bernd.edlinger@hotmail.de> >> Tom de Vries <tom@codesourcery.com> >> >> PR testsuite/77411 >> * c-c++-common/ubsan/object-size-9.c (f2, f3): Declare struct C >> variable >> with __attribute__((aligned(16))). > Just so I'm sure on this stuff. > > The tests exist to verify that ubsan detects the out-of-bounds writes. > ubsan isn't terminating the process, so we end up with a smashed stack? > > I fail to see how using aligned like this should consistently work. It > feels like a hack that just happens to work now. > > Would it work to break this up into distinct tests, exit()-ing from each > function rather than returning back to main? > Yes. I think how this test is designed, each function must be inlined, or it will fail anyway. It was for instance impossible to pass the ubsan test, if -fno-inline was used as RUNTESTFLAGS. Therefore it works as well, if main avoids to return and calls exit(0) instead, with a specific comment of course. See https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01985.html Bernd.
On 09/19/2016 03:08 PM, Bernd Edlinger wrote: >> >> Would it work to break this up into distinct tests, exit()-ing from each >> function rather than returning back to main? >> > > Yes. I think how this test is designed, each function must be inlined, > or it will fail anyway. It was for instance impossible to pass the > ubsan test, if -fno-inline was used as RUNTESTFLAGS. Presumably the dg-skip-if is ensuring that we're only testing with -O2 turned on. > > Therefore it works as well, if main avoids to return and calls > exit(0) instead, with a specific comment of course. > > See https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01985.html That works for me. jeff
Fix object-size-9.c with -fpic 2016-09-15 Bernd Edlinger <bernd.edlinger@hotmail.de> Tom de Vries <tom@codesourcery.com> PR testsuite/77411 * c-c++-common/ubsan/object-size-9.c (f2, f3): Declare struct C variable with __attribute__((aligned(16))). --- gcc/testsuite/c-c++-common/ubsan/object-size-9.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c index 46f1fb9..4cd8529 100644 --- a/gcc/testsuite/c-c++-common/ubsan/object-size-9.c +++ b/gcc/testsuite/c-c++-common/ubsan/object-size-9.c @@ -30,7 +30,7 @@ f1 (struct T x, int i) static struct C f2 (int i) { - struct C x; + struct C x __attribute__((aligned(16))); x.d[i] = 'z'; return x; } @@ -44,7 +44,7 @@ f2 (int i) static struct C f3 (int i) { - struct C x; + struct C x __attribute__((aligned(16))); char *p = x.d; p += i; *p = 'z';