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

Fix mismatch 'rz_cons_break_push' and 'rz_cons_break_pop' calls. #4289

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

Rot127
Copy link
Member

@Rot127 Rot127 commented Feb 23, 2024

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • 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 rizinorg/cutter#2552
Fixes rizinorg/cutter#3275

It should at least.

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 forgetten 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.

Fixes rizinorg/cutter#2552
Fixes rizinorg/cutter#3275
Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixes both problems for me. Amazing job!

@XVilka
Copy link
Member

XVilka commented Feb 23, 2024

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 XVilka merged commit a6c7864 into rizinorg:dev Feb 23, 2024
44 checks passed
@Rot127
Copy link
Member Author

Rot127 commented Feb 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem Empty disassembly no graph/diassembly/nothing when opening raspberry pi kernel firmware
3 participants