Re: [v4 1/4] d3dx9: Remove redundant parameter size check in set_constants().
2017-06-19 11:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)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. Also by putting the assert() in merge_const_set_entries() you're not checking the non-array, non-struct constants. Am I missing something? Is there something else that would trigger if e.g. a 4x4 matrix constants gets matched with a 3x4 parameter?
On 06/21/2017 10:55 PM, Matteo Bruni wrote:
2017-06-19 11:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)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. 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.
2017-06-21 22:18 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 06/21/2017 10:55 PM, Matteo Bruni wrote:
2017-06-19 11:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)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). 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.
On 06/22/2017 12:02 AM, Matteo Bruni wrote:
2017-06-21 22:18 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 06/21/2017 10:55 PM, Matteo Bruni wrote:
2017-06-19 11:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)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.
2017-06-21 23:32 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 06/22/2017 12:02 AM, Matteo Bruni wrote:
2017-06-21 22:18 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 06/21/2017 10:55 PM, Matteo Bruni wrote:
2017-06-19 11:24 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)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.
participants (2)
-
Matteo Bruni -
Paul Gofman