On 03/08/2017 12:00 PM, Richard Biener wrote:
> On Tue, Mar 7, 2017 at 5:01 PM, Martin Liška <[email protected]> wrote:
>> On 03/07/2017 03:53 PM, Richard Biener wrote:
>>> On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <[email protected]> wrote:
>>>> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>>>>> marxin <[email protected]> writes:
>>>>>
>>>>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C
>>>>>> b/gcc/testsuite/g++.dg/pr79769.C
>>>>>> new file mode 100644
>>>>>> index 00000000000..f9223db1b2d
>>>>>> --- /dev/null
>>>>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>>>>> @@ -0,0 +1,4 @@
>>>>>> +/* { dg-do compile { target { ! x32 } } } */
>>>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>>>>
>>>>> ... and again: make this x86-only.
>>>>>
>>>>> Rainer
>>>>>
>>>>
>>>> Thanks. I'm sending v2 of the patch.
>>>
>>> Hmm, not sure why we should handle REAL_CST here explicitely for example.
>>>
>>> Why not, instead of internal_error in the default: case do
>>>
>>> bounds = chkp_get_invalid_op_bounds ();
>>
>> Because chkp_get_invalid_op_bounds() returns bounds that are always valid
>> and as it's
>> security extension, I would be strict here in order to not handle something
>> that can bypass
>> the checking.
>>
>>>
>>> there? For the testcase why do we invoke chkp_find_bounds_1 on sth that is
>>> a REAL_CST for example?
>>
>> It's called when setting bounds in a call expr:
>>
>> #0 chkp_find_bounds_1 (ptr=0x7ffff6a03720, ptr_src=0x7ffff6a03720,
>> iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3734
>> #1 0x0000000000ec7c7d in chkp_find_bounds (ptr=0x7ffff6a03720,
>> iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3768
>> #2 0x0000000000ec22e1 in chkp_add_bounds_to_call_stmt (gsi=0x7fffffffd5d0)
>> at ../../gcc/tree-chkp.c:1901
>> #3 0x0000000000ec9a1a in chkp_instrument_function () at
>> ../../gcc/tree-chkp.c:4344
>> #4 0x0000000000eca4cb in chkp_execute () at ../../gcc/tree-chkp.c:4528
Yes, as you pointed out, we've got 2 issues here:
>
> So this happens for calls with mismatched arguments? I believe that
>
> if (BOUNDED_TYPE_P (type)
> || pass_by_reference (NULL, TYPE_MODE (type), type, true))
> new_args.safe_push (chkp_find_bounds (call_arg, gsi));
>
> may be misguided given pass_by_reference is not exposed at GIMPLE and thus
> for the pass_by_reference case it should fall back to
> "chkp_get_invalid_op_bounds"
> and thus eventually pass this down as a flag to chkp_find_bounds (or check
> on call_arg again).
Let's consider a VLA, we then call chkp_find_bounds for call_arg equal to
(gdb) p debug_tree(call_arg)
<ssa_name 0x7ffff69d5120
type <pointer_type 0x7ffff69c9e70
type <array_type 0x7ffff69c9d20 type <integer_type 0x7ffff688e5e8 int>
sizes-gimplified type_1 BLK size <var_decl 0x7ffff69d3360 D.1836>
unit size <var_decl 0x7ffff69d33f0 D.1837>
align 32 symtab 0 alias set -1 structural equality domain
<integer_type 0x7ffff69c9dc8>
pointer_to_this <pointer_type 0x7ffff69c9e70>>
unsigned DI
size <integer_cst 0x7ffff6876cd8 constant 64>
unit size <integer_cst 0x7ffff6876cf0 constant 8>
align 64 symtab 0 alias set -1 structural equality>
visited
def_stmt x.2_7 = x.1_20;
version 7>
where bounds result in:
_18 = _6 * 4;
x.1_20 = __builtin_alloca_with_align (_18, 32);
__bound_tmp.5_24 = __builtin_ia32_bndmk (x.1_20, _18);
which is consider fine. I don't understand how can be that broken on gimple as
it's not exposed
by GIMPLE?
>
> Note that here 'type' and call_arg may not match (asking for trouble). Plus
> 'fntype' is computed bogously (should use gimple_call_fntype).
>
> It later does sth like
>
> /* For direct calls fndecl is replaced with instrumented version. */
> if (fndecl)
> {
> tree new_decl = chkp_maybe_create_clone (fndecl)->decl;
> gimple_call_set_fndecl (new_call, new_decl);
> /* In case of a type cast we should modify used function
> type instead of using type of new fndecl. */
> if (gimple_call_fntype (call) != TREE_TYPE (fndecl))
> {
> tree type = gimple_call_fntype (call);
> type = chkp_copy_function_type_adding_bounds (type);
> gimple_call_set_fntype (new_call, type);
> }
> else
> gimple_call_set_fntype (new_call, TREE_TYPE (new_decl));
>
> but obviously if gimple_call_fntype and fndecl don't match cloning
> the mismatched decl isn't of very much help ... (it may just generate
> more mismatches with mismatching bounds...).
That second issue should be probably fixed by comparing each formal and actual
arguments,
where for a mismatch an invalid bounds should be returned? Am I right?
Martin
>
>> ...
>>
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Martin
>>
