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

[Paul Gofman]

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.

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()?