i#3324: Fixed use of uninitialised data caused by read_instruction. #3325
Conversation
johnfxgalea
changed the title
core/arch/x86/decode.c
Outdated
| @@ -1090,6 +1090,8 @@ read_instruction(byte *pc, byte *orig_pc, const instr_info_t **ret_info, | |||
| }); | |||
| #endif | |||
|
|
|||
| di->opcode = info->type; | |||
There was a problem hiding this comment.
If the decision is to set di->opcode inside read_instruction, all its callers should be changed as well, as all but decode_eflags_usage are using info->type and setting di->opcode if necessary themselves.
Since all other callers of read_instruction are using info->type for the opcode, the alternative fix is to change decode_eflags_usage to use info->type. (Ideally we could then remove di->opcode -- except there's the call to opnd_create_immed_float_for_opcode and one use when encoding.) If going this route, read_instruction's docs should then say it does not initialize the opcode field and returns the opcode in info->type. IIRC a memset to 0 was too expensive which is why this is so fragile.
There was a problem hiding this comment.
Thank you for your comment. Do you mean something like the new push? LMK if it needs adjusting.
There was a problem hiding this comment.
Yes, that is one way to go. Adding a test to exercise this path would be good too but not going to hold this up for that.
|
Travis failure looks like a known rare problem: fib-conflict #2641. Re-running that job. |
Fixes #3324 by initialising opcode field of di in read_instruction.
Without this fix, a usage error is caused when inserting cmov instructions.