Re: [v2 2/3] d3dx9: Use magic number instead of parameter tables for parameter handles.
2017-04-11 15:58 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com>
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.
On 04/13/2017 08:55 PM, Matteo Bruni wrote:
2017-04-11 15:58 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> 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.
2017-04-14 12:54 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 04/13/2017 08:55 PM, Matteo Bruni wrote:
2017-04-11 15:58 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> 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.
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 <gofmanp(a)gmail.com>:
On 04/13/2017 08:55 PM, Matteo Bruni wrote:
2017-04-11 15:58 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> 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.
2017-04-17 22:47 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
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.
participants (2)
-
Matteo Bruni -
Paul Gofman