Re: [v2 2/3] d3dx9: Use magic number instead of parameter tables for parameter handles.

[Matteo Bruni]

2017-04-11 15:58 GMT+02:00 Paul Gofman :
> Signed-off-by: Paul Gofman
> static struct d3dx_parameter *get_valid_parameter(struct d3dx9_base_effect *base, D3DXHANDLE parameter)
>  {
> - struct d3dx_parameter **handle_param = (struct d3dx_parameter **)parameter;
> - struct d3dx_parameter *handle_param = (struct d3dx_parameter *)parameter;
> - if (handle_param >= base->param_table.table && handle_param < base->param_table.table + base->param_table.count)
> -
>    return *handle_param;
> -
>
> if (handle_param && !strncmp(handle_param->magic_string, parameter_magic_string,
> -
>        sizeof(parameter_magic_string)))
> -
>    return handle_param;
>
> return get_parameter_by_name(base, NULL, parameter);
>
> }

Not new and somewhat separate from the patch, although it gets "more
interesting" with the new handles scheme: we should probably avoid the
call to get_parameter_by_name() if the effect was created with the
D3DXFX_LARGEADDRESSAWARE flag, returning NULL instead. Worth a test I
think.

[v2 2/3] d3dx9: Use magic number instead of parameter tables for parameter handles.

[Paul Gofman]

On 04/13/2017 08:55 PM, Matteo Bruni wrote:
> 2017-04-11 15:58 GMT+02:00 Paul Gofman :
> > Signed-off-by: Paul Gofman 
> >   static struct d3dx_parameter *get_valid_parameter(struct d3dx9_base_effect *base, D3DXHANDLE parameter)
> >   {
> > - struct d3dx_parameter **handle_param = (struct d3dx_parameter **)parameter;
> > - struct d3dx_parameter *handle_param = (struct d3dx_parameter *)parameter;
> > - if (handle_param >= base->param_table.table && handle_param < base->param_table.table + base->param_table.count)
> > -
> >    return *handle_param;
> > -
> >
> > if (handle_param && !strncmp(handle_param->magic_string, parameter_magic_string,
> > -
> >        sizeof(parameter_magic_string)))
> > -
> >    return handle_param;
> >
> > return get_parameter_by_name(base, NULL, parameter);
> >
> > }
>
> Not new and somewhat separate from the patch, although it gets "more
> interesting" with the new handles scheme: we should probably avoid the
> call to get_parameter_by_name() if the effect was created with the
> D3DXFX_LARGEADDRESSAWARE flag, returning NULL instead. Worth a test I
> think.

I did some quick testing of that when D3DXFX_LARGEADDRESSAWARE flag 
set, what I saw was:

- native d3dx crashes on attempt to get or set parameter using name as 
handle.

- when referencing parameter by name in IsParameterUsed, native either 
crash or returns false.

I guess native behaviour is just to treat handle as parameter 
pointer when that flag is set, without any checks. I don't achieve a 
reliable crash on parameter get and even set in our implementation 
though (possibly due to checks made in typed set's). So I think 
returning NULL if the magic number does not match as you suggest in this 
case (i. e. getting a predictable sure crash with null pointer access) 
is better.

[v2 2/3] d3dx9: Use magic number instead of parameter tables for parameter handles.

[Matteo Bruni]

2017-04-14 12:54 GMT+02:00 Paul Gofman :
> On 04/13/2017 08:55 PM, Matteo Bruni wrote:
> > 2017-04-11 15:58 GMT+02:00 Paul Gofman :
> > > Signed-off-by: Paul Gofman 
> > >   static struct d3dx_parameter *get_valid_parameter(struct
> > > d3dx9_base_effect *base, D3DXHANDLE parameter)
> > >   {
> > > - struct d3dx_parameter **handle_param = (struct d3dx_parameter
> > >
> > > **)parameter;
> > > - struct d3dx_parameter *handle_param = (struct d3dx_parameter
> > >
> > > *)parameter;
> > > - if (handle_param >= base->param_table.table && handle_param <
> > >
> > > base->param_table.table + base->param_table.count)
> > > -
> > >    return *handle_param;
> > > - if (handle_param && !strncmp(handle_param->magic_string,
> > >
> > > parameter_magic_string,
> > > -
> > >        sizeof(parameter_magic_string)))
> > > -
> > >    return handle_param;
> > >
> > > return get_parameter_by_name(base, NULL, parameter);
> > >
> > > }
> >
> > Not new and somewhat separate from the patch, although it gets "more
> > interesting" with the new handles scheme: we should probably avoid the
> > call to get_parameter_by_name() if the effect was created with the
> > D3DXFX_LARGEADDRESSAWARE flag, returning NULL instead. Worth a test I
> > think.
> I did some quick testing of that when D3DXFX_LARGEADDRESSAWARE flag set,
>
> what I saw was:
> - native d3dx crashes on attempt to get or set parameter using name as
>
> handle.
> - when referencing parameter by name in IsParameterUsed, native either crash
>
> or returns false.
> I guess native behaviour is just to treat handle as parameter pointer
>
> when that flag is set, without any checks. I don't achieve a reliable crash
> on parameter get and even set in our implementation though (possibly due to
> checks made in typed set's). So I think returning NULL if the magic number
> does not match as you suggest in this case (i. e. getting a predictable sure
> crash with null pointer access) is better.

Yes, agreed. Thanks for testing that out.

[v2 2/3] d3dx9: Use magic number instead of parameter tables for parameter handles.

[Paul Gofman]

I am going to send this patch (along with a few other improvements we 
discussed) at some point. Just if you don't mind I would move straight 
ahead to finalize shared parameters, to make the effect thing finally 
look mostly complete.

On 04/17/2017 11:40 PM, Matteo Bruni wrote:
> 2017-04-14 12:54 GMT+02:00 Paul Gofman :
> > On 04/13/2017 08:55 PM, Matteo Bruni wrote:
> > > 2017-04-11 15:58 GMT+02:00 Paul Gofman :
> > > > Signed-off-by: Paul Gofman 
> > > >    static struct d3dx_parameter *get_valid_parameter(struct
> > > > d3dx9_base_effect *base, D3DXHANDLE parameter)
> > > >    {
> > > > - struct d3dx_parameter **handle_param = (struct d3dx_parameter
> > > >
> > > > **)parameter;
> > > > - struct d3dx_parameter *handle_param = (struct d3dx_parameter
> > > >
> > > > *)parameter;
> > > > - if (handle_param >= base->param_table.table && handle_param <
> > > >
> > > > base->param_table.table + base->param_table.count)
> > > > -
> > > >    return *handle_param;
> > > > - if (handle_param && !strncmp(handle_param->magic_string,
> > > >
> > > > parameter_magic_string,
> > > > -
> > > >        sizeof(parameter_magic_string)))
> > > > -
> > > >    return handle_param;
> > > >
> > > >  return get_parameter_by_name(base, NULL, parameter);
> > > >
> > > > }
> > >
> > > Not new and somewhat separate from the patch, although it gets "more
> > > interesting" with the new handles scheme: we should probably avoid the
> > > call to get_parameter_by_name() if the effect was created with the
> > > D3DXFX_LARGEADDRESSAWARE flag, returning NULL instead. Worth a test I
> > > think.
> >  I did some quick testing of that when D3DXFX_LARGEADDRESSAWARE flag set,
> >
> > what I saw was:
> > - native d3dx crashes on attempt to get or set parameter using name as
> >
> > handle.
> > - when referencing parameter by name in IsParameterUsed, native either crash
> >
> > or returns false.
> >  I guess native behaviour is just to treat handle as parameter pointer
> >
> > when that flag is set, without any checks. I don't achieve a reliable crash
> > on parameter get and even set in our implementation though (possibly due to
> > checks made in typed set's). So I think returning NULL if the magic number
> > does not match as you suggest in this case (i. e. getting a predictable sure
> > crash with null pointer access) is better.
>
> Yes, agreed. Thanks for testing that out.

[v2 2/3] d3dx9: Use magic number instead of parameter tables for parameter handles.

[Matteo Bruni]

2017-04-17 22:47 GMT+02:00 Paul Gofman :
> I am going to send this patch (along with a few other improvements we
> discussed) at some point. Just if you don't mind I would move straight ahead
> to finalize shared parameters, to make the effect thing finally look mostly
> complete.

Yeah, sure, this isn't a priority by any means.