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.