IDA Hex-Rays: How to SAFELY fix incorrect function declarations?

Question

Update:

The problem turned out to be more complex and complex because Hex-Rays incorrectly restores the stack after calls of stdcall functions from cdecl functions:

.text:00403F2F 074                 mov     edx, gameScreenHeight
.text:00403F35 074                 mov     ecx, [eax]
.text:00403F37 **074**             push    10h
.text:00403F39 078                 push    edx
.text:00403F3A 07C                 mov     edx, gameScreenWidth
.text:00403F40 07C                 push    edx
.text:00403F41 080                 push    eax
.text:00403F42 084                 mov     eax, [ecx+54h]
.text:00403F45 084                 call    eax
.text:00403F47 **070**             test    eax, eax
.text:00403F49 070                 jz      short loc_4

As a result, a function that takes 4 arguments gets only three:

Bad: lpDD->lpVtbl->SetDisplayMode(lpDD, gameScreenWidth, gameScreenHeight, 16, **a1**)

Good: lpDD->lpVtbl->SetDisplayMode(lpDD, gameScreenWidth, gameScreenHeight, 16)

As a result, the function may not use the register value, but only save and restore it, but due to the shifted pointer to the stack after the call, it is considered to be used in some problem call. This causes the entire call chain to be marked as using this register as an argument.

Worst of all, such a problem arises in branched functions, which have several exit points and in each of them the stack is balanced (000). I can't just change the stack pointer after an erroneous call. I must also find another call and balance the changes made. x.x

---

The original question: I need to detect and safely fix incorrectly recognized function signatures. How I can do that?

For example, this function saves game settings:

BOOL __thiscall sub_410640(HKEY this)
{
  HKEY v1; // ecx
  HKEY v2; // ecx

  sub_431C00(this, "volumeMaster", *(_DWORD *)&phkResult);
  sub_431C00(*(HKEY *)&g_volumeMusic, "volumeMusic", *(_DWORD *)&g_volumeMusic);
  sub_431C00(v1, "volumeFX", *(_DWORD *)&g_volumeFX);
  sub_431C00(v2, "volumeSpeech", *(_DWORD *)&g_volumeSpeech);
  return sub_431C00(*(HKEY *)&dword_4A262C, "volumeMinimum", *(_DWORD *)&dword_4A262C);
}

.text:00410640     sub_410640      proc near               ; CODE XREF: PlayVideo+50↑p
.text:00410640                                             ; sub_416910+1A6↓p
.text:00410640 000                 mov     eax, phkResult
.text:00410645 000                 push    eax             ; Data
.text:00410646 004                 push    offset ValueName ; "volumeMaster"
.text:0041064B 008                 call    sub_431C00

.text:00410650 008                 mov     ecx, g_volumeMusic
.text:00410656 008                 push    ecx             ; Data
.text:00410657 00C                 push    offset aVolumemusic ; "volumeMusic"
.text:0041065C 010                 call    sub_431C00

.text:00410661 010                 mov     edx, g_volumeFX
.text:00410667 010                 push    edx             ; Data
.text:00410668 014                 push    offset aVolumefx ; "volumeFX"
.text:0041066D 018                 call    sub_431C00

.text:00410672 018                 mov     eax, g_volumeSpeech
.text:00410677 018                 push    eax             ; Data
.text:00410678 01C                 push    offset aVolumespeech ; "volumeSpeech"
.text:0041067D 020                 call    sub_431C00

.text:00410682 020                 mov     ecx, dword_4A262C
.text:00410688 020                 push    ecx             ; Data
.text:00410689 024                 push    offset aVolumeminimum ; "volumeMinimum"
.text:0041068E 028                 call    sub_431C00

.text:00410693 028                 add     esp, 28h
.text:00410696 000                 retn
.text:00410696     sub_410640      endp

If we look at the function inside, we can see that the arguments passed through the registers are not used in any way.

In addition, it is obvious that calls to the same function should be uniform and the transfer of such arguments does not make any sense.

BOOL __usercall sub_431C00@<eax>(HKEY a1@<ecx>, LPCSTR lpValueName, ...)
{
  LONG v3; // esi
  HKEY phkResult; // [esp+0h] [ebp-4h]
  va_list Data; // [esp+Ch] [ebp+8h]

  va_start(Data, lpValueName);
  phkResult = a1;
  if ( RegOpenKeyExA(HKEY_LOCAL_MACHINE, SubKey, 0, 1u, &phkResult) )
    return 0;
  v3 = RegSetValueExA(phkResult, lpValueName, 0, 4u, (const BYTE *)Data, 4u);
  RegCloseKey(phkResult);
  return v3 == 0;
}

.text:00410600     sub_410600      proc near               ; CODE XREF: sub_4073F0+2B0↑p
.text:00410600                                             ; WinMain(x,x,x,x)+5F4↓p ...
.text:00410600
.text:00410600     arg_0           = dword ptr  4
.text:00410600
.text:00410600 000                 mov     eax, [esp+arg_0]
.text:00410604 000                 test    eax, eax
.text:00410606 000                 mov     ecx, 1
.text:0041060B 000                 mov     dword_4AE978, ecx
.text:00410611 000                 mov     dword_4AF074, eax
.text:00410616 000                 jz      short locret_410632
.text:00410618 000                 mov     dword_4A267C, 69h
.text:00410622 000                 mov     dword_4AE920, 0
.text:0041062C 000                 mov     dword_4AF03C, ecx
.text:00410632
.text:00410632     locret_410632:                          ; CODE XREF: sub_410600+16↑j
.text:00410632 000                 retn
.text:00410632     sub_410600      endp

And now I need to adjust the declaration of these functions so that it corresponds to reality.

But two questions arise:

1. How to understand what the problem is?
2. How to make corrections safely so that one error in one function does not lead to unbalance of the stack and decompilation errors throughout the entire application database?

Expected result:

BOOL __usercall sub_431C00(LPCSTR lpValueName, _DWORD value);

sub_431C00("volumeMaster", *(_DWORD *)&phkResult);
sub_431C00("volumeMusic", *(_DWORD *)&g_volumeMusic);
sub_431C00("volumeFX", *(_DWORD *)&g_volumeFX);
sub_431C00("volumeSpeech", *(_DWORD *)&g_volumeSpeech);

Link to this PE File

Oh yeah, the funny thing is that in this case IDA correctly defines the function signature, but for some reason Hex-Rays blows the roof off:

IDA: int __cdecl sub_431C00(LPCSTR lpValueName, BYTE Data)

Hex-Rays: BOOL __usercall sub_431C00@<eax>(HKEY a1@<ecx>, LPCSTR lpValueName, ...)

Answer 1

Welcome to RE.SE!

How to understand what the problem is?

When the function is decompiled, IDA checks for its dependencies via dataflow analysis. Basically, it asks which values have to be defined in order to make this function work.

When it encounters the first line of the function (push ecx) IDA noticed that the value of ecx is used before it was defined in this function, hence it establishes it as a dependency, even though IDA is well aware that it is not a parameter to the RegOpenkeyExA call through its signature. At either and of the function, the ecx value is restored.

This is what makes IDA think, ecx is part of the function signature: it actually is

Fun IDA fact: When you decompile function sub_410640 first without looking at sub_431C00, the signatures are like you want them to be (due to the lack of information)

int sub_410640()
{
  sub_431C00("volumeMaster", phkResult);
  sub_431C00("volumeMusic", dword_4D55C8);
  sub_431C00("volumeFX", dword_4D55CC);
  sub_431C00("volumeSpeech", dword_4D55D0);
  return sub_431C00("volumeMinimum", dword_4A262C);
}

The core problem is the weird useage of the ecx register in this function. Even from an optimization point of view, it does not make any sense.

How to make corrections safely so that one error in one function does not lead to unbalance of the stack and decompilation errors throughout the entire application database?

For example, you could NOP-out the code pushing and poping ecx in the function, since it seems the calling function doesn't rely on this value remaining constant. I tested it real quick and it seems to work.

You could also solve the pointer aliasing problem. If you could find a way to proof that the ecx value on the stack is not written or read, one could omit this parameter. Sadly, that's pretty hard.

You could also try and come up with a better decompiler utilizing different heuristics, I guess. You could also try RetDec or Snowman for better results, but they will perform dataflow analysis similar to Hex-Rays and may end up with similar results.

The decompiler works kind of like a one-way function. When you invoke it, the current function and meta data like variable names and function signatures are passed to it. Although it allows for some corrections, its only the meta data and should not change the assembly code at all.

Answer 2

The decompiler decided that ecx is used by sub_431C00 because of push ecx at the beginning of the function which fills the stack slot later used by the variable phkResult, so it may look as if the initial value of phkResult is taken from ecx:

.text:00431C00                 push    ecx
.text:00431C01                 lea     eax, [esp+4+phkResult]
.text:00431C04                 push    eax             ; phkResult
.text:00431C05                 push    1               ; samDesired
.text:00431C07                 push    0               ; ulOptions
.text:00431C09                 push    offset SubKey   ; "SOFTWARE\\Valkyrie Studios\\Septerra Co"...
.text:00431C0E                 push    80000002h       ; hKey
.text:00431C13                 call    ds:RegOpenKeyExA
.text:00431C19                 test    eax, eax
.text:00431C1B                 jz      short loc_431C21
.text:00431C1D                 xor     eax, eax
.text:00431C1F                 pop     ecx
.text:00431C20                 retn

In fact, this push ecx is just a shorter versions of sub esp, -4 - the value of ecx does not need to be preserved across function calls so this push is used to reserve 4 bytes of the stack for the phkResult variable. Unfortunately, it is pretty difficult for an automatic algorithm to differentiate between genuine pushes to save registers or pass arguments to function calls and dummy ones to manipulate the stack. If you decided that the ecx usage is false positive, just fix the function prototype (Y key) and remove the unnecessary argument:

int __cdecl sub_431C00(LPCSTR lpValueName, DWORD Data)
{
  LSTATUS v3; // esi
  HKEY phkResult; // [esp+0h] [ebp-4h]

  if ( RegOpenKeyExA(HKEY_LOCAL_MACHINE, "SOFTWARE\\Valkyrie Studios\\Septerra Core", 0, 1u, &phkResult) )
    return 0;
  v3 = RegSetValueExA(phkResult, lpValueName, 0, REG_DWORD, (const BYTE *)&Data, 4u);
  RegCloseKey(phkResult);
  return v3 == 0;
}

And now the parent sub has nice decompilation too:

int sub_410640()
{
  sub_431C00("volumeMaster", g_volumeMaster);
  sub_431C00("volumeMusic", g_volumeMusic);
  sub_431C00("volumeFX", g_volumeFX);
  sub_431C00("volumeSpeech", g_gvolumeSpeech);
  return sub_431C00("volumeMinimum", g_volumeMinimum);
}

So, to summarize:

1. The problem is caused by compiler optimization confusing the decompiler into deciding that the register value is used while it's just a dummy filler.
2. There is no one true solution that always works, you'll need to experiment and be prepared to roll back and try again. With experience it will become easier.

P.S. IDA did not add ecx to the function prototype because it only analyzes stack arguments. The decompiler does dataflow analysis so it can recover register arguments too, but sometimes it can result in false positives, like here.