Re: [PATCH 1/2] wined3d: Correctly parse SM4 dcl_immediateConstantBuffer opcode.
On 3 December 2015 at 11:04, Józef Kucia <jkucia(a)codeweavers.com> wrote:
Signed-off-by: Józef Kucia <jkucia(a)codeweavers.com> --- This fixes infinite loop when dcl_immediateConstantBuffer is parsed.
Unfortunately, I had to reindent whole wined3d_sm4_opcode enum in order to make space for WINED3D_SM4_OP_DCL_IMMEDIATE_CONSTANT_BUFFER. I was also considering to use a shorter name: WINED3D_SM4_OP_DCL_IMMEDIATE_BUFFER. --- It's fine, if perhaps unfortunate. You could do something like WINED3D_SM4_OP_DCL_IMM_CB if we really want something shorter, but it's probably best to just follow the opcode name.
+ WINED3D_SM4_OP_MAX = 0x34, + WINED3D_SM4_OP_DCL_IMMEDIATE_CONSTANT_BUFFER = 0x35, + WINED3D_SM4_OP_MOV = 0x36, This is odd. I'd expect a DCL_ opcode to be in the > 0x58 range. Still, there's a hole there in the normal range, and "min" and "max" are also reversed compared to what you'd expect, so I suppose it's at least plausible.
opcode_token = *(*ptr)++; opcode = opcode_token & WINED3D_SM4_OPCODE_MASK; - len = ((opcode_token & WINED3D_SM4_INSTRUCTION_LENGTH_MASK) >> WINED3D_SM4_INSTRUCTION_LENGTH_SHIFT) - 1; + + if (opcode == WINED3D_SM4_OP_DCL_IMMEDIATE_CONSTANT_BUFFER) + len = *(*ptr) - 1; + else + len = ((opcode_token & WINED3D_SM4_INSTRUCTION_LENGTH_MASK) >> WINED3D_SM4_INSTRUCTION_LENGTH_SHIFT) - 1;
I don't think this is the right place to handle this. You should be able to handle this the same way as the other DCL_ opcodes. It should probably also have an entry in the opcode_info table. (Compare e.g. 6d948e1a8cdd8a9a5ae2b6124468275cbf13f2b8.)
This patch meant to be a minimal patch to prevent the infinitive loop. It's mostly for bug 39153 which is "implement enough D3D11 to run Tomb Raider 2013 without disabling d3d11 again" ;-) It allows the game to crash properly instead of looping indefinitely. It crashes because of some unsupported texture format with this patch. In order to make the patch small I haven't added WINED3DSIH_ entry for this opcode yet. Otherwise, I would have to reindent opcode tables also in shader.c, arb_program_shader.c and glsl_shader.c. The subject should actually be "wined3d: Correctly calculate length for SM4 dcl_immediateConstantBuffer opcode." On Thu, Dec 3, 2015 at 4:48 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
opcode_token = *(*ptr)++; opcode = opcode_token & WINED3D_SM4_OPCODE_MASK; - len = ((opcode_token & WINED3D_SM4_INSTRUCTION_LENGTH_MASK) >> WINED3D_SM4_INSTRUCTION_LENGTH_SHIFT) - 1; + + if (opcode == WINED3D_SM4_OP_DCL_IMMEDIATE_CONSTANT_BUFFER) + len = *(*ptr) - 1; + else + len = ((opcode_token & WINED3D_SM4_INSTRUCTION_LENGTH_MASK) >> WINED3D_SM4_INSTRUCTION_LENGTH_SHIFT) - 1;
I don't think this is the right place to handle this. You should be able to handle this the same way as the other DCL_ opcodes. It should probably also have an entry in the opcode_info table. (Compare e.g. 6d948e1a8cdd8a9a5ae2b6124468275cbf13f2b8.)
I think I still need some additional code to handle this case before len is added to *ptr. WINED3D_SM4_INSTRUCTION_LENGTH is 0 for DCL_IMMEDIATE_CONSTANT_BUFFER opcodes. In the result we are adding 4294967295 to the pointer. I could use other type than DWORD so it would effectively decrement pointer pointer also on 64 bit but I doubt we want that. I could also check if len is 0 before decrementing it but it is not right for other opcodes than DCL_IMMEDIATE_CONSTANT_BUFFER.
On 4 December 2015 at 13:06, Józef Kucia <joseph.kucia(a)gmail.com> wrote:
This patch meant to be a minimal patch to prevent the infinitive loop. It's mostly for bug 39153 which is "implement enough D3D11 to run Tomb Raider 2013 without disabling d3d11 again" ;-) It allows the game to crash properly instead of looping indefinitely. It crashes because of some unsupported texture format with this patch. In order to make the patch small I haven't added WINED3DSIH_ entry for this opcode yet. Otherwise, I would have to reindent opcode tables also in shader.c, arb_program_shader.c and glsl_shader.c. The subject should actually be "wined3d: Correctly calculate length for SM4 dcl_immediateConstantBuffer opcode."
But it doesn't actually allow the application to run, right? There's certainly something to say for limiting the scope of patches during the code freeze, but that's not necessarily the same as limiting the number of lines you touch. I think that for d3d10+ the potential for regressions is fairly limited at the moment, so in that regard adding the WINED3DSIH_ entry wouldn't concern me much. On the other hand, since (as far as I can tell) the patch doesn't make a substantial difference for the application in question, it could just as well be argued that the patch shouldn't go in at all during the code freeze.
I think I still need some additional code to handle this case before len is added to *ptr. WINED3D_SM4_INSTRUCTION_LENGTH is 0 for DCL_IMMEDIATE_CONSTANT_BUFFER opcodes. In the result we are adding 4294967295 to the pointer. I could use other type than DWORD so it would effectively decrement pointer pointer also on 64 bit but I doubt we want that. I could also check if len is 0 before decrementing it but it is not right for other opcodes than DCL_IMMEDIATE_CONSTANT_BUFFER. You're probably right. Still, it all seems pretty suspicious. For every other instruction so far the length is properly set. I suppose the issue is that the maximum size of immediate constant buffers (4096) is larger than the maximum value of the instruction length field. What does the actual bytecode look like? Any unrecognized flags?
On Fri, Dec 4, 2015 at 2:56 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
But it doesn't actually allow the application to run, right? There's certainly something to say for limiting the scope of patches during the code freeze, but that's not necessarily the same as limiting the number of lines you touch. I think that for d3d10+ the potential for regressions is fairly limited at the moment, so in that regard adding the WINED3DSIH_ entry wouldn't concern me much. On the other hand, since (as far as I can tell) the patch doesn't make a substantial difference for the application in question, it could just as well be argued that the patch shouldn't go in at all during the code freeze.
Yes, let's defer that until after the code freeze.
You're probably right. Still, it all seems pretty suspicious. For every other instruction so far the length is properly set. I suppose the issue is that the maximum size of immediate constant buffers (4096) is larger than the maximum value of the instruction length field. What does the actual bytecode look like? Any unrecognized flags?
It doesn't seem to have any unrecognized flags which could indicate that the instruction length field should be ignored. I'll check it more carefully when preparing a new version of this patch set after the code freeze.
participants (2)
-
Henri Verbeet -
Józef Kucia