rizinorg / rizin

UNIX-like reverse engineering framework and command-line toolset.
https://rizin.re
GNU Lesser General Public License v3.0
2.7k stars 361 forks source link

Rizin cannot guess the calling convention correctly #2078

Open AntonAngeloff opened 2 years ago

AntonAngeloff commented 2 years ago

Work environment

Questions Answers
OS/arch/bits (mandatory) Windows 10 / x86 / 64 bit
File format of the file you reverse (mandatory) PE
Architecture/bits of the file (mandatory) x86/32
rizin -v full output, not truncated (mandatory) rizin 0.4.0-git @ windows-x86-32 commit: 3c2e4bf63a1094c0c0901e3a13f92c1c0c92fefe, build: ?? 07.12.2021 ?.__ 1:48:41,79

Expected behavior

The correct calling convention should be inferred.

Actual behavior

When analyzing x86_32 PE binaries produced by MSVC (with C++ classes), rizin will mistakenly assume the class method calling convention to be cdecl while it is actually thiscall.

It's possible this misleads the user and the decompiler.

Steps to reproduce the behavior

The following output suggests the calling convention is cdecl (but based on the disassembly one can see ECX is treated as an argument, so it is actually thiscall):

#
offset: 0x00403ff0
name: fcn.00403ff0
size: 114
...
call-convention: cdecl
...

Test program source code

#include <iostream>

using namespace std;

class A {
public:
    A(int f) :
        m_field(f)
    {
    }

    void call_all(int a, int b) {
        method1(a, b);
        method2(a, b);
        method3(a, b);
    }

    void method1(int a, int b) {
        std::cout << "method1() -> " << m_field << ", " << a << ", " << b << "\n";
    }

    virtual void method2(int a, int b) {
        std::cout << "method2() -> " << m_field << ", " << a << ", " << b << "\n";
    }

    static void method3(int a, int b) {
        std::cout << "method3() -> " << a << ", " << b << "\n";
    }

private:
    int m_field = 0;
};

int main()
{
    cout << "starting..." << endl;
    A a(1);
    a.call_all(2, 3);

    return 0;
}
AntonAngeloff commented 2 years ago

Let me know if it's worth adding this as a test case.