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

[SharedCache] Reconsider use of BeginUndoActions / ForgetUndoActions during shared cache loading / analysis #6325

Open
bdash opened this issue Jan 16, 2025 · 1 comment
Assignees
Labels
Component: DSC Issue needs changes to the DyldSharedCacheView Effort: Trivial Issue should take < 1 day Impact: Medium Issue is impactful with a bad, or no, workaround

Comments

@bdash
Copy link
Contributor

bdash commented Jan 16, 2025

Version and Platform (required):

  • Binary Ninja Version: 4.3-dev (current HEAD)
  • OS: macOS
  • OS Version: 15.1.1
  • CPU Architecture: arm64

Bug Description:
The dyld shared cache plug-in contains a couple of locations where it does work that it brackets with BeginUndoActions / ForgetUndoActions, such as:

auto id = m_dscView->BeginUndoActions();
if (auto loadedSymbol = m_dscView->GetSymbolByAddress(symbolLocation))
{
if (m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation))
m_dscView->DefineUserSymbol(new Symbol(FunctionSymbol, prefix + loadedSymbol->GetFullName(), targetLocation));
else
m_dscView->DefineUserSymbol(new Symbol(loadedSymbol->GetType(), prefix + loadedSymbol->GetFullName(), targetLocation));
}
else if (auto sym = m_dscView->GetSymbolByAddress(symbolLocation))
{
if (m_dscView->GetAnalysisFunction(m_dscView->GetDefaultPlatform(), targetLocation))
m_dscView->DefineUserSymbol(new Symbol(FunctionSymbol, prefix + sym->GetFullName(), targetLocation));
else
m_dscView->DefineUserSymbol(new Symbol(sym->GetType(), prefix + sym->GetFullName(), targetLocation));
}
m_dscView->ForgetUndoActions(id);
and:
auto id = m_data->BeginUndoActions();
m_symbolQueue->Process();
m_data->EndBulkModifySymbols();
delete m_symbolQueue;
m_data->ForgetUndoActions(id);

From discussion on Slack, undo actions are tracked at the BinaryView level. Forgetting undo actions will forget actions performed outside of the local code, such as simultaneous operations performed by the user or by code running on other threads.

Additionally, BeginUndoActions posts work to the main thread and waits for it to complete. This can pose a performance issue if the main thread is not idle.

It would be worth revisiting what the use of BeginUndoActions / ForgetUndoActions is trying to accomplish and seeing if there's an alternative solution that doesn't have these drawbacks (perhaps auto sections / symbols rather than user?).

Slack discussions that touch on this:

https://binaryninja.slack.com/archives/C1E616ZSQ/p1734975321394819
https://binaryninja.slack.com/archives/C1E616ZSQ/p1735840209591709

@xusheng6 xusheng6 added the Component: DSC Issue needs changes to the DyldSharedCacheView label Jan 21, 2025
@plafosse
Copy link
Member

The problem here is that its defining a user symbol and not an auto symbol. I believe we can safely change this to auto symbols and then just remove the calls to Begin/ForgetUndoActions. This will also make things a lot faster.

@plafosse plafosse added Impact: Medium Issue is impactful with a bad, or no, workaround Effort: Trivial Issue should take < 1 day labels Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: DSC Issue needs changes to the DyldSharedCacheView Effort: Trivial Issue should take < 1 day Impact: Medium Issue is impactful with a bad, or no, workaround
Projects
None yet
Development

No branches or pull requests

4 participants