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 multiple crashes related to incorrect rz_iterator usage. #3308

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

karliss
Copy link
Member

@karliss karliss commented Feb 26, 2024

RzAnalysisBytes are owned by the iterator, so returning analysis bytes while destroying iterator owning it is incorrect.

Your checklist for this pull request

Detailed description

RzAnalysisBytes are owned by the iterator, so returning analysis bytes while destroying iterator owning it is incorrect. Previous version could cause use after free or attempts to free struct which shouldn't freed crashing either way.

Easiest methods to trigger the crash was opening right click context menu in disassembly or graph. And opening xrefs window on instruction with code xrefs (like the start of function). There might be others getRzAnalysisBytesSingle is indirectly used by quite a few places.

I am not too happy about the RzIter wrapper. But since it's already there I was thinking I could replace some of the places currently dealing with RzAnalysisBytes to use RzAnalysisOp instead, but not in this pull request.

Seems like 8574f0b is also included in v2.3.3 so there is a high chance that it is mostly unusable due to right click crash.

Test plan (required)

  • right click in disassembly panel -> shouldn't crash
  • right click in function graph -> shouldn't crash
  • click on function start and open xrefs window -> shouldn't crash

Closing issues

RzAnalysisBytes are owned by the iterator, so returning analysis bytes while destroying iterator owning it is incorrect.
@XVilka
Copy link
Member

XVilka commented Feb 27, 2024

Thanks, my bad for the wrong implementation at the first place. Yes, we should make a new patch release including this fix along with couple fixes in Rizin once this is merged.

@karliss karliss merged commit 6b660e7 into rizinorg:dev Feb 27, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants