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

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

Hmm.....,

On Tue, 24 Sep 2013 20:06:51, Martin Jambor wrote:
> Hi,
>
> On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote:
>> On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Hi,
>>>
>>> with the attached patch the read-side of the out of bounds accesses are fixed.
>>> There is a single new test case pr57748-3.c that is derived from Martin's test case.
>>> The test case does only test the read access and does not depend on part 1 of the
>>> patch.
>>>
>>> This patch was boot-strapped and regression tested on x86_64-unknown-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?
>>
>> Index: gcc/expr.c
>> ===================================================================
>> --- gcc/expr.c (revision 202778)
>> +++ gcc/expr.c (working copy)
>> @@ -9878,7 +9878,7 @@
>> && modifier != EXPAND_STACK_PARM
>> ? target : NULL_RTX),
>> VOIDmode,
>> - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
>> + EXPAND_MEMORY);
>>
>> /* If the bitfield is volatile, we want to access it in the
>> field's mode, not the computed mode.
>>
>> context suggests that we may arrive with EXPAND_STACK_PARM here
>> which is a correctness modifier (see its docs). But I'm not too familiar
>> with the details of the various expand modifiers, Eric may be though.

"  EXPAND_STACK_PARM is used when expanding to a TARGET on  the stack for
   a call parameter.  Such targets require special care as we haven't yet
   marked TARGET so that it's safe from being trashed by libcalls.  We
   don't want to use TARGET for anything but the final result;
   Intermediate values must go elsewhere.   Additionally, calls to
   emit_block_move will be flagged with BLOCK_OP_CALL_PARM."

This should not be a problem: If modifier == EXPAND_STACK_PARM
the target is NOT passed down, and thus not giving the modifier either
should not cause incorrect code. But I might be wrong...

On the other hand, in the case where tem is a unaligned MEM_REF and
exp is not exactly the same object as tem,
EXPAND_STACK_PARM may exactly do the wrong thing.

What I can tell is, that this patch does not change a single bit in the complete
GCC boot strap process, stage 2 and stage 3 are binary identical with or
without this patch, and that all test cases pass, even the ADA test cases...


>> That said, I still believe that fixing the misalign path in expand_assignment
>> would be better than trying to avoid it. For this testcase the issue is
>> again that expand_assignment passes the wrong mode/target to the
>> movmisalign optab.
>
> Perhaps I'm stating the obvious, but this is supposed to address a
> separate bug in the expansion of the RHS (as opposed to the first bug
> which was in the expansion of the LHS), and so we would have to make
> expand_assignment to also examine potential misalignments in the RHS,
> which it so far does not do, despite its complexity.

Thanks for pointing that out. This is true.

> Having said that, I also dislike the patch, but I am quite convinced
> that if we allow non-BLK structures with zero sized arrays, the fix
> will be ugly - but I'll be glad to be shown I've been wrong.
>
> Martin

I could of course look at the type of tem, and only mess with the
expand modifier in case of a MEM_REF. But I am not sure what
to do about the VIEW_CONVERT_EXPR, if the code at normal_inner_ref
is wrong, the code at VIEW_CONVERT_EXPR can't possibly be any better,
although I have not yet any idea how to write a test case for this:

        /* ??? We should work harder and deal with non-zero offsets.  */
        if (!offset
            && (bitpos % BITS_PER_UNIT) == 0
            && bitsize>= 0
            && compare_tree_int (TYPE_SIZE (type), bitsize) == 0)
          {
            /* See the normal_inner_ref case for the rationale.  */
            orig_op0
              = expand_expr (tem,
                             (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
                              && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
                                  != INTEGER_CST)
                              && modifier != EXPAND_STACK_PARM
                              ? target : NULL_RTX),
                             VOIDmode,
                             modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);

Especially for EXPAND_STACK_PARM? If this does really allocate
something on a register, then the result will be thrown away, because
it is not MEM_P?
Right?


Note the RHS-Bug can also affect ordinary unions on ARM and all
STRICT_ALIGNMENT targets.

Please check the attached test case. It may not be obvious at
first sight, but the assembler code for check is definitely wrong,
because it reads the whole structure, and uses only one byte of it.
And that is, not OK because x was volatile...

check:
    @ args = 0, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    stmfd    sp!, {r3, lr}
    mov    r3, #-2147483648
    ldr    r3, [r3, #2]    @ unaligned
    mov    r3, r3, lsr #24
    cmp    r3, #3
    ldmeqfd    sp!, {r3, pc}
    bl    abort


Bernd.

Attachment: pr57748-4.c
Description: Text document