-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Decompiler erroneously swapping operators without changing the operand in floating-point comparisons where data is marked constant (x86) #6528
Comments
The decompiler rule `subflow_convert` would sometimes swap the inputs to the P-Code ops `FLOAT_LESS` and `FLOAT_LESSEQUAL` if the float that was traced happened to be the second input of the operation, because the transformed operation had its inputs hardcoded: the traced float would always be the first input. While this also affected `FLOAT_EQUAL` and `FLOAT_NOTEQUAL`, it does not matter in those cases, because swapping the inputs for those operations is still logically equivalent. Fixes NationalSecurityAgency#6528.
The decompiler rule `subflow_convert` would sometimes swap the inputs to the P-Code ops `FLOAT_LESS` and `FLOAT_LESSEQUAL` if the float that was traced happened to be the second input of the operation, because the transformed operation had its inputs hardcoded: the traced float would always be the first input. While this also affected `FLOAT_EQUAL` and `FLOAT_NOTEQUAL`, it does not matter in those cases, because swapping the inputs for those operations is still logically equivalent. Fixes NationalSecurityAgency#6528.
Reproducing on Ghidra 11.0.3I loaded the attached file as Here's the decompilation at step 4 (the Here, the decompiled condition for the case in which
And here's the decompilation at step 10 (the Here, the decompiled condition for the case in which
So, if the I also created two xml files for debugging the decompilation, one with the double's mutability set to fcomp_error_normal.xml Tracking down the responsible decompiler ruleLoading the "constant" file in DecompVis, we find that the 49th rule application (of rule Before the application of rule After the application of rule Finding and fixing the bug in
|
case CPUI_FLOAT_EQUAL: | |
case CPUI_FLOAT_NOTEQUAL: | |
case CPUI_FLOAT_LESS: | |
case CPUI_FLOAT_LESSEQUAL: | |
{ | |
int4 slot = op->getSlot(vn); | |
TransformVar *rvn2 = setReplacement(op->getIn(1-slot)); | |
if (rvn2 == (TransformVar *)0) return false; | |
if (rvn == rvn2) { | |
list<PcodeOp *>::const_iterator ourIter = iter; | |
--ourIter; // Back up one to our original iterator | |
slot = op->getRepeatSlot(vn, slot, ourIter); | |
} | |
if (preexistingGuard(slot, rvn2)) { | |
TransformOp *rop = newPreexistingOp(2, op->code(), op); | |
opSetInput(rop, rvn, 0); | |
opSetInput(rop, rvn2, 1); | |
terminatorCount += 1; | |
} | |
break; | |
} |
On lines 2655 and 2656, a transform operation is created, and rvn
and rvn2
are set to use slot 0
and 1
respectively, regardless of what their input slot is in the current operation. Changing this locally to use slot
and 1-slot
and then recompiling the decompiler, we find that this indeed fixes the issue - the 1.5
constant is on the left of the <
operator:
$ ./decomp_dbg
[decomp]> restore /tmp/fcomp_error_constant.xml.txt
/tmp/fcomp_error_constant.xml.txt successfully loaded: Intel/AMD 32-bit x86
[decomp]> lo fu FUN_0000000c
Function FUN_0000000c: 0x0000000c
[decomp]> decompile
Decompiling FUN_0000000c
Decompilation complete
[decomp]> print C
undefined4 FUN_0000000c(void)
{
undefined4 uVar1;
if ((byte)(1.5 < FLOAT_00000008 | (byte)((ushort)((ushort)NAN(FLOAT_00000008) << 10) >> 8) |
(byte)((ushort)((ushort)(FLOAT_00000008 == 1.5) << 0xe) >> 8)) == 0) {
uVar1 = 0xffffffff;
}
else {
uVar1 = 1;
}
return uVar1;
}
I created a PR with this change (#6540) that fixes this issue.
Describe the bug
When a float variable is being compared with a double variable using one of the FCOM instructions, if the double is made constant, the comparator will flip directions, rendering the comparison incorrect.
For example:
will become
To Reproduce
Steps to reproduce the behavior:
Expected behavior
I expected the constant to be placed last in the comparison, flipping the operands and operators as necessary to make the statement still be correct.
Screenshots
Original:
After marking the double value as constant:
Attachments
fcompError
Environment (please complete the following information):
Additional context
I've also seen the bug on Windows 10 with versions 10.3.2, 10.4, 11.0, and 11.0.2
The text was updated successfully, but these errors were encountered: