Re: [v3 3/3] d3dx9: Remove redundant parameter size check in set_constants().

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [v3 3/3] d3dx9: Remove redundant parameter size check in set_constants().

Matteo Bruni
2017-06-15 0:00 GMT+02:00 Paul Gofman <[hidden email]>:

> Signed-off-by: Paul Gofman <[hidden email]>
> ---
> v3:
>     no changes.
> ---
>  dlls/d3dx9_36/preshader.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
> index 2785ca3..0bc31cb 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];

Is there anything else covering this case though? If I'm not mistaken
such a parameter / constant would be accepted currently so just
dropping the check would potentially allow broken preshaders to read
outside of the parameter data.

Now I certainly agree that the check shouldn't be there, it should
probably be in init_set_constants_param() instead. If this is already
checked somewhere in the code then I'm okay with the patch as is.


Reply | Threaded
Open this post in threaded view
|

Re: [v3 3/3] d3dx9: Remove redundant parameter size check in set_constants().

Paul Gofman
On 06/16/2017 06:29 PM, Matteo Bruni wrote:

> 2017-06-15 0:00 GMT+02:00 Paul Gofman <[hidden email]>:
>> Signed-off-by: Paul Gofman <[hidden email]>
>> ---
>> v3:
>>      no changes.
>> ---
>>   dlls/d3dx9_36/preshader.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
>> index 2785ca3..0bc31cb 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];
> Is there anything else covering this case though? If I'm not mistaken
> such a parameter / constant would be accepted currently so just
> dropping the check would potentially allow broken preshaders to read
> outside of the parameter data.
>
> Now I certainly agree that the check shouldn't be there, it should
> probably be in init_set_constants_param() instead. If this is already
> checked somewhere in the code then I'm okay with the patch as is.
>
>
I've checked that:

     - there is no independent size for matrices and vectors during
effect parse, the size is set as: 'param->bytes = 4 * param->rows *
param->columns;' in parse_effect_typedef();

     - parameter length in bytes is not necessarily checked in matrix
and vector set functions in effect.c.

     So the check currently present in set_constants() looks really
redundant to me. Even if I am missing some case when 'bytes' can
actually be not sufficient to hold 'rows * columns' value it seems
reasonable to me to handle it during effect creation rather than in
'init_set_constants_param()'.
     I suggest to add an assert instead of error & FIXME to
'init_set_constants_param()' the same way we just did for value sizes.