This is the mail archive of the
mailing list for the GCC project.
RE: [PATCH, PR 57748] Check for out of bounds access
- From: Bernd Edlinger <bernd dot edlinger at hotmail dot de>
- To: Martin Jambor <mjambor at suse dot cz>
- Cc: Richard Biener <richard dot guenther at gmail dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 17 Sep 2013 11:14:03 +0200
- Subject: RE: [PATCH, PR 57748] Check for out of bounds access
- Authentication-results: sourceware.org; auth=none
- References: <DUB122-W396D54115D70C9FF68A8D2E43C0 at phx dot gbl>,<CAFiYyc13uxba37WySOxXf14UVJW15_DM7jggNGbR4H2cK96Eiw at mail dot gmail dot com>,<DUB122-W323D4AAAEA1AFA890BF243E43C0 at phx dot gbl>,<CAFiYyc13wC0jmZ4xELAiL5L5Sbh0yauZWViP2hTdR+H4s0iPSw at mail dot gmail dot com>,<DUB122-W4D571A91831593BA22438E43C0 at phx dot gbl>,<CAFiYyc1ECAdBJTkcziSL_f9SQyYUPY9k19QWqkN+RfLO=i2zaQ at mail dot gmail dot com>,<20130910193228 dot GE6732 at virgil dot suse>,<DUB122-W22DAEEE94B5ED3308F6967E4390 at phx dot gbl>,<CAFiYyc3AHggjJbEmZvC_fh67o+LVpTBRdwyUv8T6EhQFE2vNWQ at mail dot gmail dot com>,<DUB122-W281E996A4885309956418EE4250 at phx dot gbl>,<20130916230945 dot GK6732 at virgil dot suse>
On Tue, 17 Sep 2013 01:09:45, Martin Jambor wrote:
> 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.
> 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.
>> 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?
>> 2013-09-15 Bernd Edlinger <firstname.lastname@example.org>
>> 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.
>> 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.