fix stack alignment prevents stack analysis problem in decompiler

[Escapingbug]

This should fix issue #1345 . This happens because of the and rsp, CONSTANT instruction that aligns the stack. This prevents the stack from being analyzed because the decompiler relies on the constant propagation till RSP + OFFSET to understand local stack variables.

This also happens when analyzing Rust which also uses the and instruction to do the stack alignment.

Example before:

After:

Minimum example that reproduces the problem:

; test program for ghidra decompiler
; nasm -felf64 test.S && ld test.o

          global    _start

test_func:
    ret


          section   .text
_start:
    push rbp
    mov rbp, rsp
    push r15
    push r14
    push r13
    push r12
    push rbx
    and rsp, -0x1000 ; <-- this is that instruction
    mov rax, 0x1c000
    sub rsp, rax

    mov rax, 1
    mov qword [rsp + 0xa020], rax
    mov qword [rsp + 0xa028], rax
    lea r12, [rsp + 0xa020]
    mov rdi, r12
    mov esi, 0x1
    call test_func

    pop rbx
    pop r12
    pop r13
    pop r14
    pop r15
    pop rbp
    ret

This patch works by ignoring the and instruction's effect on RSP (which is also explained in the comments).

ALERT: I'm still new to this area, and do not fully understand what problem this patch could cause.
Suggestions are welcome to make this PR complete :)

(Also, this PR contains some of the indentation fixing of the mixing of space and tab indentation. If you think it is out of scope, I'm happy to resubmit without those fix. Although I may suggest a little fix is quite fine in this case. )

[astrelsky]

This would be much easier to review if you remove the whitespace changes.

[Escapingbug]

This would be much easier to review if you remove the whitespace changes.

This should be resolved. Hopefully one may use some tool like clang format to resolve the whitespace problem. It is really annoying BTW.

[astrelsky]

Is this the intention for the rule to apply to every spacebase or just the stack? May you test and see if you get the expected behavior when the spacebase is ram?

[Escapingbug]

Is this the intention for the rule to apply to every spacebase or just the stack? May you test and see if you get the expected behavior when the spacebase is ram?

Do you have any suggestion about what the test program should look like?
In this case it should satisfy:

• varnode is in the spacebase
• varnode needs to be input
• varnode space should be IPTR_SPACEBASE

Also, I did some thinking about this.
Assuming that the and instruction is emitted by automatic approach (i.e, compiler) instead of manual manipulation, it should imply the alignment operation. In such case, I think this rule applies.

However, if we consider the possibility of manual manipulation, it MIGHT break this rule.
In other word, I'm not sure if these constraints are strong enough to enforce the exception of manipulated (manually constructed) situation.

It would be fantastic if you could help me think of some test program. :)

[astrelsky]

My main concern is what would happen if this rule gets applied to an architecture such as mips with gp where the space base is not the stack. To be quite frank, I still don't know all that much about the decompiler and how the varnodes work. It is still quite confusing to me.

What I had in mind for a test was to simply do something like call new/malloc align it with and and then dereference and see how it decompiles. You would never see that in a real program unless the author had no idea what they were doing, since the pointer is supposed to already be aligned, but it may be a good test sample.

[Escapingbug]

Original code:

result:

But since I have to coerce the pointer to integer type to do the "and" operation, I'm not sure if this is a legit testcase.

[astrelsky]

If you do something like gcc -S main.c -o main.s it will output the x86 assembly. You can then just modify it if necessary.

You should actually use intptr_t or ptrdiff_t here since the size of long may cause a lossy conversion.

[Escapingbug]

Hopefully, I'm not mistaken your advice. Since I don't think any modification on assembly is needed in this case.

[astrelsky]

Hopefully, I'm not mistaken your advice. Since I don't think any modification on assembly is needed in this case.

Output looks good to me, I don't see anything broken. I'll try and see if I can break this, this weekend.

Edit: I wasn't able to break it either.

[jobermayr]

jobermayr/ghidra-staging@4c30773 to fix

Program received signal SIGSEGV, Segmentation fault.
0x0000000000407970 in std::vector<Varnode*, std::allocator<Varnode*> >::operator[] (this=0x50, __n=1) at /usr/include/c++/13/bits/stl_vector.h:1124
1124            return *(this->_M_impl._M_start + __n);
(gdb) bt
#0  0x0000000000407970 in std::vector<Varnode*, std::allocator<Varnode*> >::operator[] (this=0x50, __n=1) at /usr/include/c++/13/bits/stl_vector.h:1124
#1  0x0000000000407771 in PcodeOp::getIn (this=0x0, slot=1) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/op.hh:152
#2  0x0000000000652c1c in RuleStackAlignFix::applyOp (this=0xdd4630, op=0xf2dce0, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc:3824
#3  0x00000000004c3306 in ActionPool::processOp (this=0xe11cd0, op=0xf2dce0, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:837
#4  0x00000000004c3583 in ActionPool::apply (this=0xe11cd0, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:879
#5  0x00000000004c1a75 in Action::perform (this=0xe11cd0, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:317
#6  0x00000000004c24a3 in ActionGroup::apply (this=0xdc9070, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:510
#7  0x00000000004c1a75 in Action::perform (this=0xdc9070, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:317
#8  0x00000000004c24a3 in ActionGroup::apply (this=0xdd8c90, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:510
#9  0x00000000004c1a75 in Action::perform (this=0xdd8c90, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:317
#10 0x00000000004c24a3 in ActionGroup::apply (this=0xdd94c0, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:510
#11 0x00000000004c1a75 in Action::perform (this=0xdd94c0, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:317
#12 0x00000000004c24a3 in ActionGroup::apply (this=0xdba320, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:510
#13 0x00000000004c26e9 in ActionRestartGroup::apply (this=0xdba320, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:556
#14 0x00000000004c1a75 in Action::perform (this=0xdba320, data=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/action.cc:317
#15 0x00000000005c8662 in DecompileAt::rawAction (this=0xd9a5f0) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/ghidra_process.cc:313
#16 0x00000000005c75f6 in GhidraCommand::doit (this=0xd9a5f0) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/ghidra_process.cc:140
#17 0x00000000005c9415 in GhidraCapability::readCommand (sin=..., out=...) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/ghidra_process.cc:487
#18 0x00000000005c9ab2 in main (argc=1, argv=0x7ffd156ba778) at /tmp/ghidra/Ghidra/Features/Decompiler/src/decompile/cpp/ghidra_process.cc:524