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

Matteo Bruni matteo.mystral at gmail.com
Wed Oct 30 05:04:21 CDT 2019


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



More information about the wine-devel mailing list