[v4 1/4] d3dx9: Remove redundant parameter size check in set_constants().

[Matteo Bruni]

2017-06-21 22:18 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> On 06/21/2017 10:55 PM, Matteo Bruni wrote:
>>
>> 2017-06-19 11:24 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
>>>
>>> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
>>> ---
>>> v4:
>>>      - added assert() for parameter size.
>>> ---
>>>   dlls/d3dx9_36/preshader.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
>>> index 2785ca3..76d6de9 100644
>>> --- a/dlls/d3dx9_36/preshader.c
>>> +++ b/dlls/d3dx9_36/preshader.c
>>> @@ -1178,11 +1178,6 @@ static void set_constants(struct d3dx_regstore
>>> *rs, struct d3dx_const_tab *const
>>>                           param_offset = i + j * info.major;
>>>                       else
>>>                           param_offset = i * info.minor + j;
>>> -                    if (param_offset * sizeof(unsigned int) >=
>>> param->bytes)
>>> -                    {
>>> -                        WARN("Parameter data is too short, name %s,
>>> component %u.\n", debugstr_a(param->name), i);
>>> -                        break;
>>> -                    }
>>>                       out[offset] = data[param_offset];
>>>                   }
>>>               }
>>> @@ -1303,6 +1298,11 @@ static HRESULT merge_const_set_entries(struct
>>> d3dx_const_tab *const_tab,
>>>               memmove(&const_tab->const_set[index + 1],
>>> &const_tab->const_set[i],
>>>                       sizeof(*const_tab->const_set) *
>>> (const_tab->const_set_count - i));
>>>               const_tab->const_set_count -= i - index - 1;
>>> +            assert(param->bytes >=
>>> table_info[first_const->table].component_size
>>> +                    * first_const->element_count
>>> +                    * (first_const->direct_copy ?
>>> get_reg_components(first_const->table)
>>> +                    * first_const->register_count
>>> +                    : first_const->param->rows *
>>> first_const->param->columns));
>>
>> I don't think an assert() is correct here. You are comparing parameter
>> size, which eventually depends on the effect data, with the constant
>> size, which comes from the constant table and thus again the effect
>> data. In general we shouldn't assert on invalid input data.
>
> My point here that I don't see now how 'bytes' can be less than required for
> matrix / array. 'bytes' is computed during effect parse for such data and
> not read from the effect data. So the assert condition should never fire
> here regardless of effect data unless I am misssing something.

I think we are somewhat talking past each other. My point instead is
that set_constants() essentially reads data from the effect parameter
and writes it into the shader constant. Those are two separate
entities and the parameter <-> constant match is done by name only. In
general, nothing guarantees that the parameter data is large enough
for the constant and one could craft an effect file where the
assumption is violated (e.g. by setting some large value in
RegisterCount for some constants).

Now, it looks like currently that isn't an issue in practice because
of the min() in info->major_count computation i.e. a large value is
silently fixed up. I'm arguing that that's suboptimal and a WARN in
such cases would be useful.
FWIW patch 3/4 removes that kind of "safety" min() so this becomes
more of a practical issue after that.

Am I making any sense at all?

>> Also by putting the assert() in merge_const_set_entries() you're not
>> checking the non-array, non-struct constants.
>
> It is checked more strictly with the recently added assert in
> init_set_constants_param() which was intended to check value size, that's
> why I had nothing to add there.

The component size asserts? I don't think those are enough right now.