This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR64078
- From: Bernd Edlinger <bernd dot edlinger at hotmail dot de>
- To: Jeff Law <law at redhat dot com>, Tom de Vries <Tom_deVries at mentor dot com>, "Marek Polacek" <polacek at redhat dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Jakub Jelinek <jakub at redhat dot com>, "H.J. Lu" <hjl dot tools at gmail dot com>
- Date: Mon, 19 Sep 2016 21:08:26 +0000
- Subject: Re: [PATCH] Fix PR64078
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=softfail (sender IP is 10.152.4.58) smtp.mailfrom=hotmail.de; redhat.com; dkim=none (message not signed) header.d=none;redhat.com; dmarc=none action=none header.from=hotmail.de;
- References: <DUB118-W242786F48D078983D0B3DAE4550@phx.gbl> <20150907100700.GC30849@redhat.com> <DUB118-W39447996258C266A1D63AFE4540@phx.gbl> <55EF3690.8030201@redhat.com> <DUB118-W441CDEC82B86B82325CA23E4520@phx.gbl> <55F050D5.3020408@redhat.com> <DUB118-W29D540E8FAA34840CA988CE4520@phx.gbl> <20150917150026.GC27588@redhat.com> <55FAECA8.3070909@redhat.com> <DUB118-W4175AF780B5B3FD98AA47E45A0@phx.gbl> <20150917180838.GE27588@redhat.com> <7fcd8db4-af35-a0fe-fc5a-f7999d0c5fca@mentor.com> <AM4PR0701MB216213789F6D93DB003C9543E4E10@AM4PR0701MB2162.eurprd07.prod.outlook.com> <963e7657-0e40-9d73-f199-ffc709761428@mentor.com> <AM4PR0701MB2162F4D5813449A38CA01422E4E00@AM4PR0701MB2162.eurprd07.prod.outlook.com> <7a10483e-7803-05dd-8b5e-c6ebabfa7684@mentor.com> <fa3615c4-6b12-27b1-9b53-2c5c1f40b87e@mentor.com> <707f05e9-8972-d552-5165-340aad167468@redhat.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
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.