binaryninja-api icon indicating copy to clipboard operation
binaryninja-api copied to clipboard

Cannot rename of split variable if it is a saved register (__saved_reg_)

Open utkonos opened this issue 1 year ago • 6 comments

Version and Platform (required):

  • Binary Ninja Version: 4.0.4958
  • OS: macOS
  • OS Version: 14.4.1
  • CPU Architecture: x64

Bug Description: This is in reference to 0x404d90 in f169750c922fd27298748f97c1a9e2b8442fb4d2d5d85f35f61528c4df6b3718

The variable name __saved_ebx at 0x404d91 cannot seem to be changed via right click "Rename Variable...". No matter what is entered, after clicking "Accept", the name does not change.

Additionally, this variable doesn't want to be split to correct for the problem described in the Additional Information section below.

Steps To Reproduce:

  1. Open f169750c922fd27298748f97c1a9e2b8442fb4d2d5d85f35f61528c4df6b3718
  2. Goto 0x404d91
  3. Right click on __saved_ebx
  4. Select "Rename Variable..."
  5. Change to anything
  6. Click "Accept"
  7. Observe the variable did not change names.

Expected Behavior: Variable changes names

The ideal disassembly would be something like the following two:

00404d90  53                 push    ebx {__saved_ebx}
00404d91  8bdc               mov     ebx, esp {var_20}
00404dbb  53                 push    ebx {var_20}

Then even better would be:

00404d90  53                 push    ebx {__saved_ebx}
00404d91  8bdc               mov     ebx, esp {frame_ptr}
00404dbb  53                 push    ebx {frame_ptr}

Screenshots: image

Additional Information: There is something strange happening with this variable in the first place. The __saved_ebx position on the stack as well as the automatic comment at instruction 0x404d90. However, the comments at 0x404d91 and 0x404dbb appear to be incorrect. 0x404d91 establishes a frame pointer to use to reference input parameters regardless of the stack alignment that happens at 0x404d96. I think these two should be var_20, separate from __saved_ebx. Whatever is keeping this variable from changing names, appears also to be preventing this variable from being split, which seems like the way this should be handled.

Tiny nitpick: that first ebx at 0x404d90 is a counter in some of the caller functions and therefore not always 0x0. Not sure why that is there or how to get rid of it.

utkonos avatar Apr 13 '24 00:04 utkonos

V35 folkds should search for sealants antonymy assuaged paraskiing incuse to find the binary

xusheng6 avatar Apr 17 '24 03:04 xusheng6

I can reproduce this. The reason why this is happening is we are always setting the name of the variable to __saved_reg, so the new name would not take effect. And the variable split code is also rejecting variables with such names.

We probably should relax the restrictions. However, I am wondering why you are trying to rename or split that particular variable, is there a broader analysis issue that you are tyring to fix?

xusheng6 avatar Apr 17 '24 04:04 xusheng6

I do see that there are two "push ebx" instructions at 0x404d90 and 0x404dbb. They are both annotated with __saved_ebx, which is a bit problematic, since the pushed ebx are indeed at different stack offset. This itself is a bug that should better be fixed, though I am not sure whether it is causing a broader analysis issue, e.g., does it screw up the stack pointer analysis?

xusheng6 avatar Apr 17 '24 04:04 xusheng6

Take a look at my new explanation in discussion #5289. Perhaps there is some relation to this with regards to the Stack being numbered from the frame pointer in ebx and not ebp.

utkonos avatar Apr 17 '24 10:04 utkonos

Take a look at my new explanation in discussion #5289. Perhaps there is some relation to this with regards to the Stack being numbered from the frame pointer in ebx and not ebp.

Thanks for the info! I am still not quite sure why you would wish to rename or split it, but this is definitely a valid feature request. The issue title has been updated to reflect it

xusheng6 avatar Apr 18 '24 02:04 xusheng6

I am still not quite sure why you would wish to rename or split it

I think there are two separate problems here:

  1. A bug as you note here:

I do see that there are two "push ebx" instructions at 0x404d90 and 0x404dbb. They are both annotated with __saved_ebx, which is a bit problematic, since the pushed ebx are indeed at different stack offset. This itself is a bug that should better be fixed

  1. My wish to split and rename is because that annotation is incorrect. I want to split so that the annotation at 0x404d90 remains, correctly, __saved_ebx and after the split, I can rename the variable annotation at 0x404d91 and 0x404dbb to frame_ptr or something equally meaningful.

I agree that there is a bug here that needs fixing. I wonder if just fixing the bug will change the analysis and annotations in a way where my wish to split and rename is no longer needed.

Is this clearer?

utkonos avatar Apr 18 '24 12:04 utkonos