[PATCH] d3dx9: Correctly handle NULL constant state for objects on 64 bit arch.

[Paul Gofman]

Signed-off-by: Paul Gofman 
---
 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;

[Matteo Bruni]

On Mon, Oct 28, 2019 at 8:59 AM Paul Gofman  wrote:
> Signed-off-by: Paul Gofman
>
> 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...

[Paul Gofman]

On 10/30/19 12:26, Matteo Bruni wrote:
> On Mon, Oct 28, 2019 at 8:59 AM Paul Gofman  wrote:
> > Signed-off-by: Paul Gofman
> >
> > 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()?

[Matteo Bruni]

On Wed, Oct 30, 2019 at 10:49 AM Paul Gofman  wrote:
> On 10/30/19 12:26, Matteo Bruni wrote:
> > On Mon, Oct 28, 2019 at 8:59 AM Paul Gofman  wrote:
> > > Signed-off-by: Paul Gofman
> > >
> > > 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.

[Paul Gofman]

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?

[Matteo Bruni]

On Wed, Oct 30, 2019 at 11:22 AM Paul Gofman  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.

[Piotr Caban]

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

[Matteo Bruni]

On Wed, Oct 30, 2019 at 12:23 PM Piotr Caban  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!