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

[Paul Gofman]

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?
>
> 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.
>
There is: "assert(param->bytes / (param->rows * param->columns) == 
sizeof(unsigned int));". I mean that if 'bytes' is less than required to 
hold columns * rows values, assert will fail. It apparently does not 
check 'bytes' against constant length though. The check I mean above 
should cover all the const_set's of course.