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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

Hi,

On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
>
> On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hi,
>>
>>> Eh ... even
>>>
>>> register struct { int i; int a[0]; } asm ("ebx");
>>>
>>> works. Also with int a[1] but not with a[2]. So just handling trailing
>>> arrays makes this case regress as well.
>>>
>>> Now I'd call this use questionable ... but we've likely supported that
>>> for decades and cannot change that now.
>>>
>>> Back to fixing everything in expand.
>>>
>>> Richard.
>>>
>>
>> Ok, finally you asked for it.
>>
>> Here is my previous version of that patch again.
>>
>> I have now added a new value "EXPAND_REFERENCE" to the expand_modifier
>> enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
>> constant values.
>>
>> I have done the same modification to VIEW_CONVERT_EXPR too, because
>> this is a possible inner reference, itself. It is however inherently hard to
>> test around this code.
>>
>> To understand this patch it is good to know what type of object the
>> return value "tem" of get_inner_reference can be.
>>
>> From the program logic at get_inner_reference it is clear that the
>> return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
>> ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
>> be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
>> further restricted because exp is gimplified.
>>
>> Usually the result will be a MEM_REF or a SSA_NAME of the memory where
>> the structure is to be found.
>>
>> When you look at where EXPAND_MEMORY is handled you see it is special-cased
>> in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
>> ARRAY_RANGE_REF.
>>
>> At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
>> same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
>> If it is an unaligned memory, we just return the unaligned reference.
>>
>> This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case,
>> because it is only a problem for STRICT_ALIGNMENT targets, and even there it will
>> certainly be really hard to find test cases that exercise this code.
>>
>> In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
>> we do not have to touch the handling of the outer modifier. However we pass
>> EXPAND_REFERENCE to the inner object, which should not be a recursive
>> use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
>>
>> TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
>> EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
>>
>>
>> Boot-strapped and regression-tested on x86_64-linux-gnu
>> OK for trunk?
>
> You point to a weak spot in expansion - that it handles creating
> the base MEM to offset with handled components by recursing
> into the case that handles bare MEM_REFs. This makes the
> bare MEM_REF handling code somewhat awkward (it's the
> one to assign mem-attrs which are later adjusted for example).
>
> Maybe a better appropach than adding yet another expand
> modifier would be to split out the "base MEM" expansion part
> out of the bare MEM_REF handling code so we can call that
> instead of recursing.
>
> In this light - instead of a new expand modifier don't you want
> an actual flag that specifies we are coming from a call that
> wants to expand a base? That is, allow EXPAND_SUM
> but with the recursion flag set?
>

I think you are right. After some thought, I start to like that idea.

This way we have at least much more flexibility, how to handle the inner
references correctly, and if I change only  the interfaces of expand_expr_real/real_1
that will not be used at too many places, either.

> Finally I think the recursion into the VIEW_CONVERT_EXPR case
> is only there because of the keep_aligning flag of get_inner_reference
> which should be obsolete now that we properly handle its effects
> in get_object_alignment. So you wouldn't need to adjust this path
> if we finally can get rid of that.
>

I proposed a patch for that, but did not hear from you:
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html

nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner
reference, the code there should be kept in sync with the normal_inner_ref,
and MEM_REF, IMHO.

Attached, my updated patch for PR57748, Part 2.

Boot-strapped and regression-tested on X86_64-pc-linux-gnu.

Ok for trunk?


Thanks
Bernd.

> What do others think?
>
> Thanks,
> Richard.
>
>> Thanks
>> Bernd.