This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH, PR 57748] Check for out of bounds access


Hi Martin,

On Tue, 17 Sep 2013 01:09:45, Martin Jambor wrote:
> Hi,
>
> On Sun, Sep 15, 2013 at 06:55:17PM +0200, Bernd Edlinger wrote:
>> Hello Richard,
>>
>> attached is my second attempt at fixing PR 57748. This time the
>> movmisalign path is completely removed and a similar bug in the read
>> handling of misaligned structures with a non-BLKmode is fixed
>> too. There are several new test cases for the different possible
>> failure modes.
>
> I think the third and fourth testcases are undefined as the
> description of zero-length arrays extension clearly says the whole
> thing only makes sense when used as the last field of the
> outermost-aggregate type. I have not really understood what the third
> testcase is supposed to test but I did not try too much. Instead of
> the fourth testcase, you can demonstrate the need for your change in
> expand_expr_real_1 by augmenting the original testcase a little like
> in attached pr57748-m1.c.

The third test case tries to demonstrate the possible write data store
race (by checking the assembler output). But you are right, this example
is probably not valid C at all.

I was actually worried about unions with non-BLK mode and
a movmisalign optab handler.

When you look at stor-layout.c (compute_record_mode)
you'll see, that in the case of a union usually an integer mode is chosen,
which is exactly the same size as the whole union.
And just by chance this does not have a movmisalign optab.

Therefore I tried to cheat with that zero-sized array, which should
probably be rejected at stor-layout.c in the first place.

When I tried to make a test case out of it, the bug on the read side hit
me as a total surprise...

> The hunk in expand_expr_real_1 can prove problematic if at any point
> we need to pass some other modifier to the expansion of tem. I'll try
> to see if I can come up with a testcase tomorrow. But perhaps we
> never do (and can hope we never will) and then it would be sort of
> OKish (note that I cannot approve anything) even though it can
> pessimize unaligned access paths (by not using movmisalign_optab even
> when perfectly possible - which is always when there is no zero sized
> array). It really just shows how evil non-BLKmode structures with
> zero-sized arrays are and how they complicate things. The expansion
> of component_refs is reasonably built around the assumption that we'd
> expand the structure in its mode in the most efficient manner and then
> chuck the correct part out of it, but here we need to tell the
> expansion of the structure to hold itself back because we'll be
> looking outside of the structure (as specified by mode).

I too am under the very strong impression that this was not the intention
of the design to use a non-BLKmode on a structure with zero-sized arrays.

> I'm not sure to what extent the hunk adding tests for bitregion_start
> and bitregion_end being zero is connected to this issue. I do not see
> any of the testcases exercising that path. If it is indeed another
> problem, I think it should be submitted (and potentially committed) as
> a separate patch, preferably with a testcase.

Yes, you're probably right. I was unable to find a test case where this
code path executes with bitregions. As I said, it maybe possible to prove that
bitregion_start and bitregion_end == 0 if the other conditions are satisfied.
What is obvious, that it would cause problems to set bitpos=0 when
bitregion_start/end is pointing elsewhere.
It is however much easier to prove that not going into that code path
would not cause any problems if bitregion_start/end is not zero.

So this was just for safer programming, but probably no real bug.


Thanks,
Bernd.

> Having said all that, I think that removing the misalignp path from
> expand_assignment altogether is a good idea. I have verified that
> when the expander is now presented with basically the same thing that
> 4.7 choked on, expand_expr (..., EXPAND_WRITE) can cope with it (see
> attached file c.c) and doing that simplifies this complex code path.
>
> Thanks,
>
> Martin
>
>>
>> This patch was boot-strapped and regression tested on  x86_64-unknown-linux-gnu
>> and i686-pc-linux-gnu.
>>
>> Additionally I generated eCos and an eCos-application (on ARMv5 using packed
>> structures) with an arm-eabi cross compiler, and looked for differences in the
>> disassembled code with and without this patch, but there were none.
>>
>> OK for trunk?
>>
>> Regards
>> Bernd.
>
>> 2013-09-15 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> PR middle-end/57748
>> * expr.c (expand_assignment): Remove misalignp code path.
>> Check for bitregion in offset arithmetic.
>> (expand_expr_real_1): Use EXAND_MEMORY on base object.
>>
>> testsuite:
>>
>> PR middle-end/57748
>> * gcc.dg/torture/pr57748-1.c: New test.
>> * gcc.dg/torture/pr57748-2.c: New test.
>> * gcc.dg/torture/pr57748-3.c: New test.
>> * gcc.dg/torture/pr57748-3a.c: New test.
>> * gcc.dg/torture/pr57748-4.c: New test.
>> * gcc.dg/torture/pr57748-4a.c: New test.
>>
>
> 		 	   		  

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]