[PATCH] d3dx9: Correctly handle NULL constant state for objects on 64 bit arch.
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- dlls/d3dx9_36/effect.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 7d686a435d..5be2725d3c 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -973,6 +973,8 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta BOOL update_all, BOOL *param_dirty) { struct d3dx_parameter *param = &state->parameter; + enum STATE_CLASS state_class; + static void *null_ptr; *param_value = NULL; *out_param = NULL; @@ -981,13 +983,30 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta switch (state->type) { case ST_PARAMETER: - param = state->referenced_param; + *out_param = param = state->referenced_param; + *param_value = param->data; *param_dirty = is_param_dirty(param, pass->update_version); - /* fallthrough */ + return D3D_OK; + case ST_CONSTANT: *out_param = param; - *param_value = param->data; + + if (((state_class = state_table[state->operation].class) == SC_VERTEXSHADER + || state_class == SC_PIXELSHADER || state_class == SC_TEXTURE) + && param->bytes < sizeof(void *)) + { + *param_value = &null_ptr; + + if (param->type != D3DXPT_INT || *(unsigned int *)param->data) + FIXME("Unexpected parameter for object, param->type %u, param->class %u.\n", + param->type, param->class); + } + else + { + *param_value = param->data; + } return D3D_OK; + case ST_ARRAY_SELECTOR: { unsigned int array_idx; -- 2.21.0
On Mon, Oct 28, 2019 at 8:59 AM Paul Gofman <gofmanp(a)gmail.com> wrote:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- dlls/d3dx9_36/effect.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 7d686a435d..5be2725d3c 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -973,6 +973,8 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta BOOL update_all, BOOL *param_dirty) { struct d3dx_parameter *param = &state->parameter; + enum STATE_CLASS state_class; + static void *null_ptr;
*param_value = NULL; *out_param = NULL; @@ -981,13 +983,30 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta switch (state->type) { case ST_PARAMETER: - param = state->referenced_param; + *out_param = param = state->referenced_param; + *param_value = param->data; *param_dirty = is_param_dirty(param, pass->update_version); - /* fallthrough */ + return D3D_OK; + case ST_CONSTANT: *out_param = param; - *param_value = param->data; + + if (((state_class = state_table[state->operation].class) == SC_VERTEXSHADER + || state_class == SC_PIXELSHADER || state_class == SC_TEXTURE) + && param->bytes < sizeof(void *)) + { + *param_value = &null_ptr; + + if (param->type != D3DXPT_INT || *(unsigned int *)param->data) + FIXME("Unexpected parameter for object, param->type %u, param->class %u.\n", + param->type, param->class);
How does this happen? The effect somehow uses D3DXPT_INT instead of e.g. D3DXPT_VERTEXSHADER and tries to get away with it? What effects / games did you see doing that? OT, right now I'm having weird issues with the 64-bit mingw build: for some reason the (only) sprintf() call in effect.c always returns just "[", which then breaks a bunch of things. I assume it's some issue on my side but if anyone has any idea I'm all ears...
On 10/30/19 12:26, Matteo Bruni wrote:
On Mon, Oct 28, 2019 at 8:59 AM Paul Gofman <gofmanp(a)gmail.com> wrote:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- dlls/d3dx9_36/effect.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 7d686a435d..5be2725d3c 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -973,6 +973,8 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta BOOL update_all, BOOL *param_dirty) { struct d3dx_parameter *param = &state->parameter; + enum STATE_CLASS state_class; + static void *null_ptr;
*param_value = NULL; *out_param = NULL; @@ -981,13 +983,30 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta switch (state->type) { case ST_PARAMETER: - param = state->referenced_param; + *out_param = param = state->referenced_param; + *param_value = param->data; *param_dirty = is_param_dirty(param, pass->update_version); - /* fallthrough */ + return D3D_OK; + case ST_CONSTANT: *out_param = param; - *param_value = param->data; + + if (((state_class = state_table[state->operation].class) == SC_VERTEXSHADER + || state_class == SC_PIXELSHADER || state_class == SC_TEXTURE) + && param->bytes < sizeof(void *)) + { + *param_value = &null_ptr; + + if (param->type != D3DXPT_INT || *(unsigned int *)param->data) + FIXME("Unexpected parameter for object, param->type %u, param->class %u.\n", + param->type, param->class); How does this happen? The effect somehow uses D3DXPT_INT instead of e.g. D3DXPT_VERTEXSHADER and tries to get away with it? What effects / games did you see doing that?
When the object state is initialized by NULL constant, state constant parameter gets D3DXPT_INT type and, of course, 4 byte size. As far as I am aware this can happen with constant set states only, if it involves parameter modifiable from the outside it always has _OBJECT type as expected. We have such an example in tests, see effect in test_effect_null_shader(). The state is not applied in the test, but if I apply the states chances to get a crash are low, while the effect from that test has exactly this case. I spotted real crashes due to this problem when testing "X Rebirth" (see https://bugs.winehq.org/show_bug.cgi?id=47974), which is 64 bit and has NULL shaders in effects. But the crash is not easily reproduced there also, while it can happen sometimes (the 4 bytes after zero 4 bytes usually happen to be 0 too). In fact, I could reliably reproduce that with the bunch of my local years old incomplete patches with various optimizations, some of which including tracking of parameters to states correspondance and doing a lot of HeapAlloc(), which probably distracts the actual memory layout. Regarding the fix, the alternate solution would be to alloc minimum 8 bytes for such a parameter data (check this in _parse_state()). That would avoid extra runtime checks. The only reason I did not go this way at once is that we previously did not alter parameter definitions upon effect parse, leaving all the interpretation to the apply state (while not much of that interpretation was required). But in this case maybe it would be a better solution.
OT, right now I'm having weird issues with the 64-bit mingw build: for some reason the (only) sprintf() call in effect.c always returns just "[", which then breaks a bunch of things. I assume it's some issue on my side but if anyone has any idea I'm all ears...
Yes, I had exactly the same and worked that locally by using snprintf() instead of sprintf(). I did not say anything about it yet as I thought it is specific to my mingw version or machine setup (e. g., we don't see test failures on testbot). It breaks a lot of effect tests on x86_64 bit. I also did not debug what exactly is wrong with sprintf(). I was using mingw 6.0 from Fedora repositories. Since it happens I am not the only one with this problem, maybe we can change sprintf() to snprintf()?
On Wed, Oct 30, 2019 at 10:49 AM Paul Gofman <gofmanp(a)gmail.com> wrote:
On 10/30/19 12:26, Matteo Bruni wrote:
On Mon, Oct 28, 2019 at 8:59 AM Paul Gofman <gofmanp(a)gmail.com> wrote:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- dlls/d3dx9_36/effect.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c index 7d686a435d..5be2725d3c 100644 --- a/dlls/d3dx9_36/effect.c +++ b/dlls/d3dx9_36/effect.c @@ -973,6 +973,8 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta BOOL update_all, BOOL *param_dirty) { struct d3dx_parameter *param = &state->parameter; + enum STATE_CLASS state_class; + static void *null_ptr;
*param_value = NULL; *out_param = NULL; @@ -981,13 +983,30 @@ static HRESULT d3dx9_get_param_value_ptr(struct d3dx_pass *pass, struct d3dx_sta switch (state->type) { case ST_PARAMETER: - param = state->referenced_param; + *out_param = param = state->referenced_param; + *param_value = param->data; *param_dirty = is_param_dirty(param, pass->update_version); - /* fallthrough */ + return D3D_OK; + case ST_CONSTANT: *out_param = param; - *param_value = param->data; + + if (((state_class = state_table[state->operation].class) == SC_VERTEXSHADER + || state_class == SC_PIXELSHADER || state_class == SC_TEXTURE) + && param->bytes < sizeof(void *)) + { + *param_value = &null_ptr; + + if (param->type != D3DXPT_INT || *(unsigned int *)param->data) + FIXME("Unexpected parameter for object, param->type %u, param->class %u.\n", + param->type, param->class); How does this happen? The effect somehow uses D3DXPT_INT instead of e.g. D3DXPT_VERTEXSHADER and tries to get away with it? What effects / games did you see doing that?
When the object state is initialized by NULL constant, state constant parameter gets D3DXPT_INT type and, of course, 4 byte size. As far as I am aware this can happen with constant set states only, if it involves parameter modifiable from the outside it always has _OBJECT type as expected. We have such an example in tests, see effect in test_effect_null_shader(). The state is not applied in the test, but if I apply the states chances to get a crash are low, while the effect from that test has exactly this case. I spotted real crashes due to this problem when testing "X Rebirth" (see https://bugs.winehq.org/show_bug.cgi?id=47974), which is 64 bit and has NULL shaders in effects. But the crash is not easily reproduced there also, while it can happen sometimes (the 4 bytes after zero 4 bytes usually happen to be 0 too). In fact, I could reliably reproduce that with the bunch of my local years old incomplete patches with various optimizations, some of which including tracking of parameters to states correspondance and doing a lot of HeapAlloc(), which probably distracts the actual memory layout.
Cool. Okay, I guess we have to cope with it.
Regarding the fix, the alternate solution would be to alloc minimum 8 bytes for such a parameter data (check this in _parse_state()). That would avoid extra runtime checks. The only reason I did not go this way at once is that we previously did not alter parameter definitions upon effect parse, leaving all the interpretation to the apply state (while not much of that interpretation was required). But in this case maybe it would be a better solution.
I had a sentence mentioning that idea at some point in the draft. I guess that's preferable if it doesn't turn too ugly.
OT, right now I'm having weird issues with the 64-bit mingw build: for some reason the (only) sprintf() call in effect.c always returns just "[", which then breaks a bunch of things. I assume it's some issue on my side but if anyone has any idea I'm all ears...
Yes, I had exactly the same and worked that locally by using snprintf() instead of sprintf(). I did not say anything about it yet as I thought it is specific to my mingw version or machine setup (e. g., we don't see test failures on testbot). It breaks a lot of effect tests on x86_64 bit. I also did not debug what exactly is wrong with sprintf(). I was using mingw 6.0 from Fedora repositories. Since it happens I am not the only one with this problem, maybe we can change sprintf() to snprintf()?
Oh, nice, so it isn't just me. I'm more concerned that this might break a lot more than just d3dx9, git grep shows a ton of sprintf() in Wine and then you have all the applications using our msvcrt. I haven't spent a ton of time on it either but if someone wants to have a look (*cough* Piotr *cough*) reproducing it is just a matter of running the d3dx9:effect test. The sprintf() in question is at line 5068 of dlls/d3dx9/effect.c in current master.
On 10/30/19 13:04, Matteo Bruni wrote:
I had a sentence mentioning that idea at some point in the draft. I guess that's preferable if it doesn't turn too ugly.
I will do that, hope it will not be ugly. It just pulls another thing: I am probably supposed to use heap_alloc() / heap_free() now in the new code, but to do so all the existing allocations should be swithced to that first. I think I will have to do that first. Or do you think maybe it does not worth touching the whole d3dx9 code for that purpose now, and just use one more HeapReAlloc()? Or is it considered acceptable to mix helper and direct WINAPI allocations / frees?
On Wed, Oct 30, 2019 at 11:22 AM Paul Gofman <gofmanp(a)gmail.com> wrote:
On 10/30/19 13:04, Matteo Bruni wrote:
I had a sentence mentioning that idea at some point in the draft. I guess that's preferable if it doesn't turn too ugly.
I will do that, hope it will not be ugly. It just pulls another thing: I am probably supposed to use heap_alloc() / heap_free() now in the new code, but to do so all the existing allocations should be swithced to that first. I think I will have to do that first. Or do you think maybe it does not worth touching the whole d3dx9 code for that purpose now, and just use one more HeapReAlloc()? Or is it considered acceptable to mix helper and direct WINAPI allocations / frees?
I'm fine with mixing for the time being.
Hi Matteo, I've sent a fix for ntdll sprintf. We should probably introduce ucrtbase headers at some point that provide inline sprintf function (or add sprintf to import lib). Fixing ntdll implementation should be good enough for now. Cheers, Piotr
On Wed, Oct 30, 2019 at 12:23 PM Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Matteo,
I've sent a fix for ntdll sprintf.
We should probably introduce ucrtbase headers at some point that provide inline sprintf function (or add sprintf to import lib). Fixing ntdll implementation should be good enough for now.
Cheers, Piotr
Oh, I didn't realize that was ntdll's sprintf(). Also, that was scary fast. Thanks!
participants (3)
-
Matteo Bruni -
Paul Gofman -
Piotr Caban