Wrong variables name substitution?

[alex-curiou]

Work environment

Questions	Answers
OS/arch/bits (mandatory)	Ubuntu 20.04.1
File format of the file you reverse (mandatory)	ELF
Architecture/bits of the file (mandatory)	arm64-v8a
r2 -v full output, not truncated (mandatory)	radare2 4.6.0-git 25649 @ linux-x86-64 git.4.4.0-678-g5ccf9fd48
commit: 5ccf9fd4824ee54e999e92993442ba200c65ab80 build: 2020-09-13__02:36:27

Expected behavior

No variable substitution should happen at all in the third line. This behaviour, btw, is also in line with IDA.

; var int64_t var_0h_2 @ sp+0x20
sub sp, sp, 0x30
stp x29, x30, [sp, 0x20]
add x29, sp, 0x20

Actual behavior

; var int64_t var_0h_2 @ sp+0x20
sub sp, sp, 0x30
stp x29, x30, [sp, 0x20]
add x29, var_0h_2x20

Steps to reproduce the behavior

$ r2 -AA libnative-lib.so
> s 0x00015780
> pi 3

Binary to test

Provided in https://github.com/radareorg/radare2/issues/17637#issuecomment-692018942

Additional Logs, screenshots, source-code, configuration dump, ...

[ret2libc]

@alex-curiou Could you provide the exact binary where this happen? It would help reproducing the issue, thanks.

[alex-curiou]

Here is the binary, so for example: r2 -AA libnative-lib.so s 0x00015780

NOTE: I've reported not correct ABI, so changed from arm to arm64-v8a

libnative-lib.zip

[ret2libc]

@trufae there is probably something wrong in parse_arm_pseudo.c.

The original instruction is:

add x29, sp, 0x20 # x29 = sp + 0x20

So, if I get it right, x29 will be the address of var_0h_2. Given that, I'm not even sure you want to see var_0h_2 there at all, it seems confusing. What do you think?

[trufae]

yes this is wrong. and this is what IDA does. and this is what i was complaining some days ago because of a change in the disasm. The substituion is wrong because it should even replace the add for a mov to make it meaningful that way.

Obviously having the var that is accessed is nice, but i think it shouldnt be shown that way. It was shown as a comment when using esil before iirc. but its always better to have this replaced inside the instruction. but i dont think this is a good way to render that. And ive been always trying to avoid mimic IDA's disasm which is full of lies like this that make me feel confused when comparing disassemblers

[alex-curiou]

That is not a canonical way of showing disassembly, so it is a bit hard to understand and will lead to creation of a our own dialect. So we will have a bit more closed community ... From another side, may be some one like it this way and we can have it like an option? But not default one. So people don't be scary when they have it 'out-of-the-box' =))

[trufae]

An option for what? this disassembly is wrong. it should be fixed. i was stating that IDA is doing things like this, and im against inventing instructions. I dont think r2 community is closed at all. I dont want such thing, not even as an option. because its misleading.

[ret2libc]

Could someone provide a snapshot of what is IDA showing exactly?

[alex-curiou]

Here is how IDA 7.0 shows that funciton

But I think this way + variables usage in comments is better and more clear and easier to read

After all, this is not pseudocode ...

[ret2libc]

Ok, so add x29, sp, 0x20 is not converted to anything special in IDA either. It is just r2 that incorrectly substitutes things. Then there is definitely a bug in parse_arm_pseudo.c. @alex-curiou would you be willing to give this issue a try with a PR?

[alex-curiou]

In the coming several weeks I have really busy days (( if only I suddenly find a solution earlier for the current issue I'm busy with...

[ret2libc]

@alex-curiou no problem ;)

I took the liberty to update the description of the issue.

[alex-curiou]

Of course, You are welcome ))

[trufae]

i have started to work on fixing this issue but its breaking abi, so wont merge the PRs until we start the abi-breaking stage, there are several more issues related to variable analysis right now, so im trying to collect them to have tests for all of them