[v7 2/7] d3dx9: Implement preshader parsing.

[Paul Gofman]

On 03/29/2016 02:16 AM, Matteo Bruni wrote:
>
>> +static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc)
>> +{
>> +    while ((*ptr & 0xffff) == 0xfffe)
>> +    {
>> +        if (*(ptr + 1) == fourcc)
>> +            return ptr + 2;
>> +        ptr += 1 + (*ptr >> 16);
>> +    }
>> +    return NULL;
>> +}
> This is probably paranoid but in theory a broken or malicious effect
> could send you rummaging outside of the bytecode buffer so all the
> offsets should be checked. I don't think it's critical in practice
> i.e. I'll probably sleep just fine if you decide to ignore this
> issue...
I will pass through bytecode size parameter and add a size check. There
is D3DXGetShaderConstantTable function used in another function taking
the same bytecode reference and doing comment search in a similar way,
and it does not have size parameter, so it will crash anyway on insane
bytecode. The same is for d3d shader create functions. But those
functions do not modify the memory at bytecode, and segfault on read
access cannot be used alone to inject anything. At the same time
parse_preshader writes to that pointer a 0xfffe0000 constant (restoring
a memory a bit later) to make it suitable D3DXGetShaderConstantTable, so
actually adding these checks does not seem much paranoid.

>
>
> It depends on what is coming up next but maybe it would make sense to
> add an enum for "PRES_REGTAB_IMMED + 1" (incidentally,
> PRES_REGTAB_FIRST is never used in this patch series).
PRES_REGTAB_FIRST was used in tables free function. I now removed
_FIRST/_LAST, used 0/_COUNT instead, and introduced
PRES_REGTAB_FIRST_SHADER to use it in parse_param_eval instead of
'PRES_REGTAB_IMMED + 1'. Please note that I introduced
PRES_REGTAB_FIRST/_LAST in v3 patch series based on comments on v1, and
should issue a possible endless loop warning :). I suggest that if there
is still any concern regarding this enum maybe we could confirm its
final form at this point? The current enum after my last edits looks
like this:

enum pres_reg_tables
{
    PRES_REGTAB_IMMED, /* immediate double constants from CLIT */
    PRES_REGTAB_CONST,
    PRES_REGTAB_OCONST,
    PRES_REGTAB_OBCONST,
    PRES_REGTAB_OICONST,
    PRES_REGTAB_TEMP,
    PRES_REGTAB_COUNT,
    PRES_REGTAB_FIRST_SHADER = PRES_REGTAB_CONST,
};

>
> +        if (FAILED(regstore_alloc_table(&peval->pres.regs, i)))
> +        {
> +            hr = E_OUTOFMEMORY;
> +            goto err_out;
> +        }
> +    }
> +
> +    if (TRACE_ON(d3dx))
> +        dump_bytecode(byte_code, byte_code_size);
> +
> +    *peval_out = peval;
> +    TRACE("retval %u, *peval_out %p.\n", D3D_OK, *peval_out);
> Same with this. This one in particular doesn't seem worth keeping.
Maybe we can leave this in place? I changed this to TRACE("Created
param_eval %p.\n", *peval_out). I saw similar traces many times in d3d
code and they are extremely helpful when we need to find a creation log
for object we see used somewhere.