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

[Matteo Bruni]

2017-06-21 23:32 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> On 06/22/2017 12:02 AM, Matteo Bruni wrote:
>>
>> 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).
>
> Oh yes, sorry, I misunderstood what you mean because the check this patch
> removes was effectively checking parameter 'bytes' vs its rows and columns.
> This is indeed not the case anymore after patch 3/4. I will add a check (of
> 'bytes' vs count based on constant data) and a WARN with error return to
> init_set_constants_param(). Maybe it will be 2 separate checks (for scalars
> / matrices / vectors and for result of const_set's merge separately). Is it
> ok?

Yeah, the check I'm arguing for isn't quite the same as the if you're
removing in this patch and I see how all this might have been
confusing, sorry about it.

About the specific checks to introduce, those you mention sound good
although it will only become clear once I see the actual code.