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

BinaryView Memory Leak in DebuggerController When a ReportCollection Tab is Closed Last #641

Open
bpotchik opened this issue Oct 19, 2024 · 3 comments
Assignees
Milestone

Comments

@bpotchik
Copy link
Member

  1. open file
  2. open report widget (current_function.request_debug_report("mlil"))
  3. close the tab/file for that view
  4. close the report widget tab
  5. get_memory_usage_info()
@xusheng6 xusheng6 transferred this issue from Vector35/binaryninja-api Oct 29, 2024
@xusheng6 xusheng6 added this to the Frogstar milestone Oct 29, 2024
@xusheng6 xusheng6 self-assigned this Oct 29, 2024
@xusheng6
Copy link
Member

xusheng6 commented Oct 29, 2024

This is how the relevant events unfold:

  1. when we close the tab for the view itself, the debugger actually correctly detects the closing of the last tab that belongs to the view, and destroys the controller for it
  2. The report widget automatically becomes the current tab
  3. The report widget has a reference to the binary view, and SetCurrentBinaryView is called with it
  4. The Python console receives it, and constructs a new DebuggerController using that view
  5. When the report widget is closed, NotificationListener::OnBeforeCloseFile is not called, so the debugger does not get a chance to know about
  6. Memory leak

I do not know what is a proper fix for this because:

  1. The main windows seems to intentionally NOT call OnBeforeCloseFile for widget like report widget, settings widget. Which I think is a wise choice
  2. One might argue that the closing of the view tab is NOT technically the last tab of the view (the report widget is still open and it has a reference to the view). But that does not help, since even if we do not destroy the old controller when that happens, we still would not be able to know the reprot tab is finally being closed at all.

To summarize, the debugger relies on OnBeforeCloseFile to know when a file is closed and then cleanup the controller for it. Now the report widget is the last tab that gets closed, and OnBeforeCloseFile is NOT called for it.

I also believe that this also affects the database saving logic in the very same way, since the way the debugger detects the "last open tab" of the file is the same as how main window does it. If we modify the view, and then open the report widget, we will be asked to save the database when the view tab is closed. However, you can still make changes to the view using the Python console, and the main window will not ask you to save it

@bpotchik
Copy link
Member Author

One option to solve this is to parent any report widgets to the view tab. When the last view tab is closed, all other report tabs with that view are closed first.

@xusheng6 xusheng6 removed their assignment Oct 30, 2024
@xusheng6 xusheng6 modified the milestones: Frogstar, Gallifrey Oct 30, 2024
@xusheng6
Copy link
Member

I am bumping this since this is not really a regression -- it has been there for a while and we happen to discover it recently

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

No branches or pull requests

2 participants