GrammaTech / ddisasm

A fast and accurate disassembler
https://grammatech.github.io/ddisasm/
GNU Affero General Public License v3.0
663 stars 62 forks source link

Incorrect disassembly of binary 'FADD ST(n1) ST(n2)' instructions #76

Closed niscove closed 5 months ago

niscove commented 6 months ago

REVISION 1.2.840.113556.1.4.145 downloaded windows dev executable dated April 12 2024

Ddisasm interprets the binary sequences

d8 c1 as fadd ST(1), ST(0) d8 c2 as fadd ST(2), ST(0)

The correct outputs should instead be

d8 c1 fadd ST(0), ST(1) d8 c2 fadd ST(0), ST(2)

.

junghee commented 6 months ago

Hi @niscove, thank you for reporting the issue.

In fadd st(1), there is an implicit operand st(0), and the semantics is that it performs the addition of the value in st(1) to the value in st(0) and stores the result in st(0), right?

I can see that ddisasm produces fadd st(1) for the instruction in the output asm, which seems to be correct.

When you say that Ddisasm interprets the binary as fadd st(1), st(0), do you mean that Ddisasm treats st(1) as the destination, and st(0) as the source?

niscove commented 6 months ago

The attached file has it all. Without getting into detail, ddisasm should read d8 c1 and output fadd ST(0),ST(1), not fadd ST(1),ST(0) and should read d8 c2 and output fadd ST(0),ST(2, not fadd ST(2),ST(0) all as in the attached file.

From: junghee @.> Sent: Thursday, May 23, 2024 4:37 PM To: GrammaTech/ddisasm @.> Cc: Iscove, Norman @.>; Mention @.> Subject: [External] Re: [GrammaTech/ddisasm] Incorrect disassembly of binary 'FADD ST(n1) ST(n2)' instructions (Issue #76)

Hi @niscovehttps://urldefense.com/v3/__https:/github.com/niscove__;!!CjcC7IQ!KXmEfJ47yD4Acjwffip25t1kc0BS2ls22ZMmEXlhyaueWNWXZSvhXfchS_Qlu7lee5ZqzQ63PUgBpeM4ysIa1GqvvxM$, thank you for reporting the issue.

In fadd st(1), there is an implicit operand st(0), and the semantics is that it performs the addition of the value in st(1) to the value in st(0) and stores the result in st(0), right?

I can see that ddisasm produces fadd st(1) for the instruction in the output asm, which seems to be correct.

When you say that Ddisasm interprets the binary as fadd st(1), st(0), do you mean that Ddisasm treats st(1) as the destination, and st(0) as the source?

junghee commented 6 months ago

Sorry, I can't find any attached file in this issue page.

niscove commented 6 months ago

This is the url for the page I sent you with detail on the FADD instruction

https://www.felixcloutier.com/x86/fadd:faddp:fiadd

From: junghee @.> Sent: Thursday, May 23, 2024 7:48 PM To: GrammaTech/ddisasm @.> Cc: Iscove, Norman @.>; Mention @.> Subject: [External] Re: [GrammaTech/ddisasm] Incorrect disassembly of binary 'FADD ST(n1) ST(n2)' instructions (Issue #76)

Sorry, I can't find any attached file in this issue page.

junghee commented 6 months ago

I assume you would have some binary that you observed the issue? If so, could you attach that?

I tried writing an assembly code with fadd instruction:

  1 #include <stdio.h>
  2
  3 int main() {
  4     float val1 = 3.14;
  5     float val2 = 2.71;
  6     float val3 = 1.62;
  7     float result;
  8
  9     __asm__ (
 10         "fld %1;"        // Load val1 into ST(0)
 11         "fld %2;"        // Load val2 into ST(0), val1 is now in ST(1)
 12         "fld %3;"        // Load val3 into ST(0), val2 is in ST(1), val1 in ST(2)
 13         "fadd %%st(1), %%st(0);"  // ST(0) = ST(0) + ST(1) (val3 + val2)
 14         //"fadd %%st(1);"  // ST(0) = ST(0) + ST(1) (val3 + val2)
 15         "fadd %%st(2);"  // ST(0) = ST(0) + ST(2) (result of previous + val1)
 16         "fstp %0;"       // Store the result from ST(0) to result and pop ST(0)
 17         : "=m" (result)
 18         : "m" (val1), "m" (val2), "m" (val3)
 19         : "st"
 20     );
 21
 22     printf("Result: %f\n", result);
 23     return 0;
 24 }

Both fadd st(1) and fadd st(1), st(0) produces the same bytes: d8 c1

For both case, Ddisasm outputs fadd st(1).

niscove commented 6 months ago

" I assume you would have some binary that you observed the issue? If so, could you attach that?"

The binary code is

d8 c1

d8 c2

When Ddisasm is asked to disassemble this binary code, it outputs

fadd ST(1),ST(0)

fadd ST(2),ST(0)

The output is wrong.

It should be

fadd ST(0),ST(1)

fadd ST(0),ST(2)

The coding is exactly detailed in the link I sent you.

From: junghee @.> Sent: Thursday, May 23, 2024 8:28 PM To: GrammaTech/ddisasm @.> Cc: Iscove, Norman @.>; Mention @.> Subject: [External] Re: [GrammaTech/ddisasm] Incorrect disassembly of binary 'FADD ST(n1) ST(n2)' instructions (Issue #76)

I assume you would have some binary that you observed the issue? If so, could you attach that?

I tried writing an assembly code with fadd instruction:

1 #include

2

3 int main() {

4 float val1 = 3.14;

5 float val2 = 2.71;

6 float val3 = 1.62;

7 float result;

8

9 asm (

10 "fld %1;" // Load val1 into ST(0)

11 "fld %2;" // Load val2 into ST(0), val1 is now in ST(1)

12 "fld %3;" // Load val3 into ST(0), val2 is in ST(1), val1 in ST(2)

13 "fadd %%st(1), %%st(0);" // ST(0) = ST(0) + ST(1) (val3 + val2)

14 //"fadd %%st(1);" // ST(0) = ST(0) + ST(1) (val3 + val2)

15 "fadd %%st(2);" // ST(0) = ST(0) + ST(2) (result of previous + val1)

16 "fstp %0;" // Store the result from ST(0) to result and pop ST(0)

17 : "=m" (result)

18 : "m" (val1), "m" (val2), "m" (val3)

19 : "st"

20 );

21

22 printf("Result: %f\n", result);

23 return 0;

24 }

Both fadd st(1) and fadd st(1), st(0) produces the same bytes: d8 c1

For both case, Ddisasm outputs fadd st(1).

junghee commented 6 months ago

Can you give me your command line for running ddisasm on your binary? I don't quite understand how you create an input binary with your binary code to feed into ddisasm.

niscove commented 6 months ago

I wrote an assembly script, 'z.asm' which begins with the lines

   .
   .
   fadd ST(0),ST(1)
   fadd ST(0),ST(2)
   pushd  CF_TEXT
   call   IsClipboardFormatAvailable
   '
   .

I compiled z.asm with MASM generating the executable file z.exe . The first lines in z.exe disassembled by x64dbg are:

00401000     6A 00               push 0
00401002     E8 25010000         call <JMP.&GetModuleHandleA>
00401007     A3 00304000         mov dword ptr ds:[403000],eax
0040100C     D8C1                **fadd st(0),st(1)**   CORRECT
0040100E     D8C2                **fadd st(0),st(2)**   CORRECT
00401010     6A 01               push 1
00401012     E8 3F010000         call <JMP.&IsClipboardFormatAvailable>
.
.

Using ddisasm on z.exe with the command line ddisasm --asm zDDout.asm z.exe produces the disassembly file zDDout.asm. The first lines are

   mov DWORD PTR [$L_403000],EAX
   **fadd ST(1),ST(0)**         WRONG
   **fadd ST(2),ST(0)**         WRONG
   push 1
   call FUN_401156
   .
   .

z.zip

junghee commented 6 months ago

Thanks for the details. We were able to reproduce the problem, and are in process of resolving it. I will let you know when it is done.

aeflores commented 5 months ago

I believe this is now solved with https://github.com/GrammaTech/gtirb-pprinter/commit/d9dc966176379e9a29e96558f28d589d00ff91a9