rizin icon indicating copy to clipboard operation
rizin copied to clipboard

Fix mismatch 'rz_cons_break_push' and 'rz_cons_break_pop' calls.

Open Rot127 opened this issue 1 year ago • 1 comments

Your checklist for this pull request

  • [x] I've read the guidelines for contributing to this repository
  • [x] I made sure to follow the project's coding style
  • [ ] I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • [ ] I've added tests that prove my fix is effective or that my feature works (if possible)
  • [ ] I've updated the rizin book with the relevant information (if needed)

Detailed description If a function calls rz_cons_break_push() but never calls rz_cons_break_pop() before return, the stack count of RzConsContext->break_stack contains too many elements (each time one too much).

This in turn will lead to not resetting RzConsContext->breaked flag. Because the flag is only set to false, if rz_stack_is_empty(context->break_stack) == true (in rz_cons_context_break_push()).

This wasn't a problem so far, because RzConsContext->breaked is simply never set to true (exceptions are some timeout cases as far as I can see). Also these cases when rz_cons_break_pop() was forgotten to be called, were edge error cases. So not often hit.

But if Rizin is usd by Cutter RzConsContext->breaked is set to true, if an AnalysisTask interrupt is handled (in AnalysisTask::interrupt()). This interrupt is triggered for example, when the introduction dialog is closed and the main Cutter window opens (after the optional aaa).

Now, if the binary file was analysed with aaa, and a lot of error cases were hit, those error cases sometimes never called rz_cons_break_pop() before returning from their function. Although, of course, they should have to the RzConsContext->break_stack is in a proper state.

This means, when the main Cutter window opens binary files which trigger many error edge cases, the RzConsContext->break_stack is not empty (because of the not executed rz_cons_break_pop()).

This also means, that the last thing done, was setting RzConsContext->breaked = true (by AnalysisTask::interrupt()).

If Cutter wants to show some disassembly, it calls rz_core_print_disasm() which checks RzConsContext == false via rz_cons_is_breaked(). This condition is never true, because the flag was not reset to false because the stack was never empty. So it returns before anything was disassembled.

Hence Cutter gets no disassembly text.

Test plan

All green

Closing issues

Fixes https://github.com/rizinorg/cutter/issues/2552 Fixes https://github.com/rizinorg/cutter/issues/3275

It should at least.

Rot127 avatar Feb 23 '24 13:02 Rot127

Long-term though, we should think how to avoid this kind of situation, since "counting" RzCons breaks every time isn't sustainable, especially if we are going to make analysis multithreaded.

XVilka avatar Feb 23 '24 13:02 XVilka

The good thing is though, that the push and pops where always contained in a single function. So if we have RzCons properly implemented with mutexes or even can synchronize it somehow over different analysis tasks, it would work. Point is, the more thought we put into RzCons, the less trouble those things make.

Rot127 avatar Feb 23 '24 18:02 Rot127