Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling rz_ghidra_fini after destroying plugin's global variables (decompiler_mutex) #2704

Closed
yshybeka opened this issue Jun 13, 2021 · 1 comment · Fixed by #2994
Closed
Assignees
Labels
BUG Decompiler Issues and feature requests related to the decompiler in Cutter

Comments

@yshybeka
Copy link

yshybeka commented Jun 13, 2021

First of all, I want to meantion that using global mutex in rz_ghidra_init/_fini looks at least suspicious to me, though the reason for it might be oc command which calls rz_core_fini followed by rz_core_init. I'm reporting this to cutter's queue as even if it can be fixed in ghidra plugin, it may happen to any other plugin using global variables, but please move it to rz-ghidra's queue if cutter's behavior is expected.

CutterCore instance being a global variable is not guaranteed to be destroyed before other global variables, and in fact it's destroyed after global variables from shared libraries at least in FreeBSD/clang. The rizin cli in contrast calls rz_core_free from main function and doesn't have any issues with rz-ghidra plugin.

This is relevant cutter's backtrace, watchpoint is set for decompiler_mutex and you can see it's called before rz_ghidra_fini using that mutex.

Watchpoint 1 hit:
old value: 0x00000008050a7008
new value: 0x0000000000000002
Process 83019 stopped
* thread #1, name = 'cutter', stop reason = watchpoint 1
    frame #0: 0x00000008034cb642 libthr.so.3`pthread_mutex_destroy + 146
libthr.so.3`pthread_mutex_destroy:
->  0x8034cb642 <+146>: movq   %fs:0x10, %rdi
    0x8034cb64b <+155>: movq   %rbx, %rsi
    0x8034cb64e <+158>: callq  0x8034cb670               ; ___lldb_unnamed_symbol67$$libthr.so.3
    0x8034cb653 <+163>: movq   %rbx, %rdi
(lldb) bt
* thread #1, name = 'cutter', stop reason = watchpoint 1
  * frame #0: 0x00000008034cb642 libthr.so.3`pthread_mutex_destroy + 146
    frame #1: 0x0000000802ffd499 libc++.so.1`std::__1::recursive_mutex::~recursive_mutex() + 9
    frame #2: 0x00000008031ed350 libc.so.7`__cxa_finalize + 320
    frame #3: 0x00000008031ed8d1 libc.so.7`exit + 33
    frame #4: 0x0000000000408eb7 cutter`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1_c.c:75:2
(lldb) c
Process 83019 resuming
Process 83019 stopped
* thread #1, name = 'cutter', stop reason = breakpoint 2.1
    frame #0: 0x00000008115d4dea core_ghidra.so`rz_ghidra_fini(core=0x000000080f0ab640) at core_ghidra.cpp:684:40
   681   
   682   static bool rz_ghidra_fini(RzCore *core)
   683   {
-> 684           std::lock_guard<std::recursive_mutex> lock(decompiler_mutex);
   685           shutdownDecompilerLibrary();
   686           return true;
   687   }
(lldb) bt
* thread #1, name = 'cutter', stop reason = breakpoint 2.1
  * frame #0: 0x00000008115d4dea core_ghidra.so`rz_ghidra_fini(core=0x000000080f0ab640) at core_ghidra.cpp:684:40
    frame #1: 0x0000000801517b1b librz_core.so.0.2.1`rz_core_plugin_fini(core=0x000000080f0ab640) at cplugin.c:21:4
    frame #2: 0x00000008014c1865 librz_core.so.0.2.1`rz_core_fini(c=0x000000080f0ab640) at core.c:2685:2
    frame #3: 0x00000008014c1bd0 librz_core.so.0.2.1`rz_core_free(c=0x000000080f0ab640) at core.c:2746:3
    frame #4: 0x0000000000439279 cutter`CutterCore::~CutterCore(this=0x0000000000798478) at Cutter.cpp:247:5
    frame #5: 0x00000000004620dc cutter`(anonymous namespace)::Q_QGS_uniqueInstance::innerFunction(this=0x0000000000798478)::Holder::~Holder() at Cutter.cpp:27:1
    frame #6: 0x00000008031ed350 libc.so.7`__cxa_finalize + 320
    frame #7: 0x00000008031ed8d1 libc.so.7`exit + 33
    frame #8: 0x0000000000408eb7 cutter`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1_c.c:75:2
(lldb) c
Process 83019 resuming
Process 83019 stopped
* thread #1, name = 'cutter', stop reason = signal SIGABRT
    frame #0: 0x00000008031c32ea libc.so.7`__sys_thr_kill + 10
libc.so.7`__sys_thr_kill:
->  0x8031c32ea <+10>: jb     0x8031c1448
    0x8031c32f0 <+16>: retq   
    0x8031c32f1:       int3   
    0x8031c32f2:       int3   
@xarkes xarkes added BUG Decompiler Issues and feature requests related to the decompiler in Cutter labels Jun 13, 2021
@thestr4ng3r
Copy link
Member

rz_ghidra_fini() is generally meant to be called any time in the code the plugin should be uninitialized and before global dtors are run.
In that sense, the root of the issue is indeed in Cutter. I think the cleanest way to solve it will be to not have CutterCore as a global object at all since that has also caused other similar issues in the past. As a nice side effect, this will also make it possible to have multiple cores in the same process.

thestr4ng3r added a commit that referenced this issue Jul 9, 2022
#2704)

Using Q_GLOBAL_STATIC meant that the CutterCore was destructed late as
part of a binary destructor. It would then free the RzCore, calling for
example the fini callbacks of all plugins. However global destructors in
shared library plugins may have already been run at this point, leading
to for example rz-ghidra's decompiler_mutex being used after
destruction.
Instead of the Q_GLOBAL_STATIC-managed global object, we are now
handling the lifetime of the CutterCore ourselves and only injecting its
instance to be accessed globally. This can also be a first step towards
making the core instance completely local.
thestr4ng3r added a commit that referenced this issue Jul 9, 2022
Using Q_GLOBAL_STATIC meant that the CutterCore was destructed late as
part of a binary destructor. It would then free the RzCore, calling for
example the fini callbacks of all plugins. However global destructors in
shared library plugins may have already been run at this point, leading
to for example rz-ghidra's decompiler_mutex being used after
destruction.
Instead of the Q_GLOBAL_STATIC-managed global object, we are now
handling the lifetime of the CutterCore ourselves and only injecting its
instance to be accessed globally. This can also be a first step towards
making the core instance completely local.
thestr4ng3r added a commit that referenced this issue Jul 9, 2022
Using Q_GLOBAL_STATIC meant that the CutterCore was destructed late as
part of a binary destructor. It would then free the RzCore, calling for
example the fini callbacks of all plugins. However global destructors in
shared library plugins may have already been run at this point, leading
to for example rz-ghidra's decompiler_mutex being used after
destruction.
Instead of the Q_GLOBAL_STATIC-managed global object, we are now
handling the lifetime of the CutterCore ourselves and only injecting its
instance to be accessed globally. This can also be a first step towards
making the core instance completely local.
thestr4ng3r added a commit that referenced this issue Jul 9, 2022
Using Q_GLOBAL_STATIC meant that the CutterCore was destructed late as
part of a binary destructor. It would then free the RzCore, calling for
example the fini callbacks of all plugins. However global destructors in
shared library plugins may have already been run at this point, leading
to for example rz-ghidra's decompiler_mutex being used after
destruction.
Instead of the Q_GLOBAL_STATIC-managed global object, we are now
handling the lifetime of the CutterCore ourselves and only injecting its
instance to be accessed globally. This can also be a first step towards
making the core instance completely local.
thestr4ng3r added a commit that referenced this issue Sep 10, 2022
Using Q_GLOBAL_STATIC meant that the CutterCore was destructed late as
part of a binary destructor. It would then free the RzCore, calling for
example the fini callbacks of all plugins. However global destructors in
shared library plugins may have already been run at this point, leading
to for example rz-ghidra's decompiler_mutex being used after
destruction.
Instead of the Q_GLOBAL_STATIC-managed global object, we are now
handling the lifetime of the CutterCore ourselves and only injecting its
instance to be accessed globally. This can also be a first step towards
making the core instance completely local.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Decompiler Issues and feature requests related to the decompiler in Cutter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants