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

SectionWidget / FeatureMap updating causes UI to be unresponsive #6169

Open
bdash opened this issue Nov 21, 2024 · 0 comments
Open

SectionWidget / FeatureMap updating causes UI to be unresponsive #6169

bdash opened this issue Nov 21, 2024 · 0 comments
Assignees
Labels
State: Awaiting Triage Issue is waiting for more in-depth triage from a developer

Comments

@bdash
Copy link
Contributor

bdash commented Nov 21, 2024

Version and Platform (required):

  • Binary Ninja Version: 4.3.6457-dev
  • OS: macOS
  • OS Version: 15.1.1
  • CPU Architecture: arm64

Bug Description:
I'm exercising dyld shared cache loading by loading a batch of images from the shared cache in sequence.

While each image loads, the Binary Ninja UI frequently becomes totally unresponsive for tens of seconds at a time. Running sample binaryninja shows that the main thread is blocked below either FeatureMap::renderAnalysisData or SectionWidget::updateInfo, trying to take a lock held by a background analysis thread.

This happens even when the feature map and section widget are not visible on screen.

Steps To Reproduce:

  1. Check out https://github.com/bdash/binaryninja-api/tree/dsc-persistent-data-structures and apply the patch below.
  2. Build and install the plug-in.
  3. Open /System/Cryptexes/OS/System/Library/dyld/dyld_shared_cache_arm64e and wait for the initial load / analysis to complete.
  4. In the image list in the Dyld Shared Cache triage view, click and drag to select 50 or so images.
  5. Press Load.

Expected Behavior:

  1. UI elements shouldn't block for an extended period of time while waiting on locks held by background threads.
  2. UI elements that aren't visible shouldn't be updating at all.

Binary:
I've been testing with the system dyld shared cache on macOS 15.1.1.

Additional Information:
Note this uses my branch of the dyld shared cache plug-in as it is significantly faster than what is currently in dev, with an additional patch applied to make it easier to load a number of images in a row.

sample binaryninja shows the main thread looks like one of the following:

    + 2373 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__  (in CoreFoundation) + 28  [0x191219d34]
    +   2373 QCocoaEventDispatcherPrivate::postedEventsSourceCallback(void*)  (in libqcocoa.dylib) + 436  [0x104b3a064]
    +     2373 QCocoaEventDispatcherPrivate::processPostedEvents()  (in libqcocoa.dylib) + 312  [0x104b390a4]
    +       2373 QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*)  (in QtCore) + 1464  [0x1051a83b0]
    +         2373 QCoreApplication::notifyInternal2(QObject*, QEvent*)  (in QtCore) + 212  [0x1051a70c4]
    +           2373 QApplication::notify(QObject*, QEvent*)  (in QtWidgets) + 3244  [0x106235298]
    +             2373 QApplicationPrivate::notify_helper(QObject*, QEvent*)  (in QtWidgets) + 272  [0x1062339a4]
    +               2373 QWidget::event(QEvent*)  (in QtWidgets) + 3656  [0x10627f230]
    +                 2373 QObject::event(QEvent*)  (in QtCore) + 616  [0x1051ea6c4]
    +                   2373 FeatureMap::renderAnalysisData()  (in libbinaryninjaui.1.dylib) + 424  [0x105965990]
    +                     2373 ???  (in libbinaryninjaui.1.dylib)  load address 0x10580c000 + 0x5cfb9c  [0x105ddbb9c]
    +                       2373 BNGetFunctionBasicBlockList  (in libbinaryninjacore.1.dylib) + 40  [0x1174b1f98]
    +                         2373 ???  (in libbinaryninjacore.1.dylib)  load address 0x116f50000 + 0x51e998  [0x11746e998]
    +                           2373 std::recursive_mutex::lock()  (in libc++.1.dylib) + 16  [0x19106a8ec]
    +                             2373 _pthread_mutex_firstfit_lock_slow  (in libsystem_pthread.dylib) + 220  [0x19112ddbc]
    +                               2373 _pthread_mutex_firstfit_lock_wait  (in libsystem_pthread.dylib) + 84  [0x1911303f8]
    +                                 2373 __psynch_mutexwait  (in libsystem_kernel.dylib) + 8  [0x1910f4a9c]
    + 2343 __CFRunLoopDoSource0  (in CoreFoundation) + 176  [0x191219cc8]
    +   2343 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__  (in CoreFoundation) + 28  [0x191219d34]
    +     1880 QCocoaEventDispatcherPrivate::postedEventsSourceCallback(void*)  (in libqcocoa.dylib) + 436  [0x104b3a064]
    +     ! 1880 QCocoaEventDispatcherPrivate::processPostedEvents()  (in libqcocoa.dylib) + 312  [0x104b390a4]
    +     !   1880 QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*)  (in QtCore) + 1464  [0x1051a83b0]
    +     !     1880 QCoreApplication::notifyInternal2(QObject*, QEvent*)  (in QtCore) + 212  [0x1051a70c4]
    +     !       1880 QApplication::notify(QObject*, QEvent*)  (in QtWidgets) + 3244  [0x106235298]
    +     !         1880 QApplicationPrivate::notify_helper(QObject*, QEvent*)  (in QtWidgets) + 272  [0x1062339a4]
    +     !           1880 QWidget::event(QEvent*)  (in QtWidgets) + 3656  [0x10627f230]
    +     !             1880 QObject::event(QEvent*)  (in QtCore) + 616  [0x1051ea6c4]
    +     !               1235 SectionWidget::updateInfo()  (in libbinaryninjaui.1.dylib) + 100  [0x105a95b20]
    +     !               : 1235 ???  (in libbinaryninjaui.1.dylib)  load address 0x10580c000 + 0x5aad04  [0x105db6d04]
    +     !               :   1235 BNGetSections  (in libbinaryninjacore.1.dylib) + 40  [0x11721a948]
    +     !               :     1235 ???  (in libbinaryninjacore.1.dylib)  load address 0x116f50000 + 0x2afc94  [0x1171ffc94]
    +     !               :       1235 std::recursive_mutex::lock()  (in libc++.1.dylib) + 16  [0x19106a8ec]
    +     !               :         1235 _pthread_mutex_firstfit_lock_slow  (in libsystem_pthread.dylib) + 220  [0x19112ddbc]
    +     !               :           1235 _pthread_mutex_firstfit_lock_wait  (in libsystem_pthread.dylib) + 84  [0x1911303f8]
    +     !               :             1235 __psynch_mutexwait  (in libsystem_kernel.dylib) + 8  [0x1910f4a9c]

In both cases they appear to be waiting on a lock held by a background thread that is taking its sweet time beneath BNAddUserSection:

    2343 Thread_6797989: Worker PRI 
    + 2343 thread_start  (in libsystem_pthread.dylib) + 8  [0x19112e0fc]
    +   2343 _pthread_start  (in libsystem_pthread.dylib) + 136  [0x1911332e4]
    +     2343 ???  (in libbinaryninjacore.1.dylib)  load address 0x116f50000 + 0xc76cd0  [0x117bc6cd0]
    +       2343 ???  (in libbinaryninjacore.1.dylib)  load address 0x116f50000 + 0xc778b0  [0x117bc78b0]
    +         2343 WorkerActionCallback(void*)  (in libsharedcacheui.dylib) + 36  [0x14516614c]  binaryninjaapi.cpp:281
    +           2343 std::__function::__func<DSCTriageView::DSCTriageView(QWidget*, BinaryNinja::Ref<BinaryNinja::BinaryView>)::$_8::operator()(bool) const::'lambda'(), void ()>::operator()()  (in libsharedcacheui.dylib) + 60  [0x145153d9c]  function.h:311
    +             2343 BNDSCViewLoadImageWithInstallName  (in libsharedcache.dylib) + 208  [0x144d8c5f4]  SharedCache.cpp:3217
    +               2343 SharedCacheCore::SharedCache::LoadImageWithInstallName(std::basic_string<char>)  (in libsharedcache.dylib) + 5128  [0x144d7efec]  SharedCache.cpp:1909
    +                 2343 SharedCacheCore::SharedCache::InitializeHeader(BinaryNinja::Ref<BinaryNinja::BinaryView>, VM*, SharedCacheCore::SharedCacheMachOHeader, std::vector<SharedCacheCore::MemoryRegion const*>)  (in libsharedcache.dylib) + 1496  [0x144d83eec]  SharedCache.cpp:2585
    +                   2343 BNAddUserSection  (in libbinaryninjacore.1.dylib) + 520  [0x11721a6ec]
    +                     2343 ???  (in libbinaryninjacore.1.dylib)  load address 0x116f50000 + 0x26b0b0  [0x1171bb0b0]
    +                       2328 ???  (in libbinaryninjacore.1.dylib)  load address 0x116f50000 + 0x144e7c  [0x117094e7c]
    +                       ! 2041 ???  (in libbinaryninjacore.1.dylib)  load address 0x116f50000 + 0x281ab8  [0x1171d1ab8]
    +                       ! : 2018 ???  (in libbinaryninjacore.1.dylib)  load address 0x116f50000 + 0x2d63e8  [0x1172263e8]
    +                       ! : | 1144 ???  (in libdebuggercore.dylib)  load address 0x11edb0000 + 0x54940  [0x11ee04940]
    +                       ! : | + 441 ???  (in libdebuggercore.dylib)  load address 0x11edb0000 + 0x5f48c  [0x11ee0f48c]
    +                       ! : | + ! 419 operator new(unsigned long)  (in libc++abi.dylib) + 52  [0x1910ed36c]

Rough patch to enable multi-selection and batch loading of images from the dyld shared cache:

diff --git a/view/sharedcache/ui/dsctriage.cpp b/view/sharedcache/ui/dsctriage.cpp
index 9f456d72..427865d8 100644
--- a/view/sharedcache/ui/dsctriage.cpp
+++ b/view/sharedcache/ui/dsctriage.cpp
@@ -644,9 +644,12 @@ DSCTriageView::DSCTriageView(QWidget* parent, BinaryViewRef data) : QWidget(pare
 	m_bottomRegionTabs = new SplitTabWidget(m_bottomRegionCollection);
 	m_bottomRegionTabs->setTabStyle(new GlobalAreaTabStyle());
 
-	auto loadImageTable = new FilterableTableView;
+	auto loadImageTable = new FilterableTableView(nullptr, false);
 	{
 		auto loadImageModel = new QStandardItemModel(0, 2, loadImageTable);
+		auto proxyModel = new QSortFilterProxyModel(loadImageTable);
+		proxyModel->setSourceModel(loadImageModel);
+
 		{
 			connect(
 				cacheBlocksView, &DSCCacheBlocksView::loadDone, [this, loadImageModel]()
@@ -668,15 +671,26 @@ DSCTriageView::DSCTriageView(QWidget* parent, BinaryViewRef data) : QWidget(pare
 		auto loadImageButton = new CustomStyleFlatPushButton();
 		{
 			connect(loadImageButton, &QPushButton::clicked,
-				[this, loadImageTable](bool) {
+				[this, loadImageTable, data](bool) {
 					auto selected = loadImageTable->selectionModel()->selectedRows();
 					if (selected.size() == 0)
 					{
 						return;
 					}
 
-					auto name = selected[0].data().toString().toStdString();
-					WorkerPriorityEnqueue([this, name]() { m_cache->LoadImageWithInstallName(name); });
+					std::vector<std::string> installNamesToLoad;
+					for (auto& item : selected) {
+						auto name = item.data().toString().toStdString();
+						installNamesToLoad.push_back(name);
+					}
+					WorkerPriorityEnqueue([this, installNamesToLoad, data]() {
+						data->SetAnalysisHold(true);
+						for (auto& name : installNamesToLoad) {
+							m_cache->LoadImageWithInstallName(name);
+						}
+						data->SetAnalysisHold(false);
+						data->UpdateAnalysis();
+					});
 				});
 			loadImageButton->setText("Load");
 
@@ -696,7 +710,7 @@ DSCTriageView::DSCTriageView(QWidget* parent, BinaryViewRef data) : QWidget(pare
 
 		connect(loadImageTable, &FilterableTableView::activated, this, [=](const QModelIndex& index)
 			{
-				auto name = loadImageModel->item(index.row(), 0)->text().toStdString();
+				auto name = proxyModel->data(index).toString().toStdString();
 				WorkerPriorityEnqueue([this, name]()
 					{
 						m_cache->LoadImageWithInstallName(name);
@@ -704,13 +718,18 @@ DSCTriageView::DSCTriageView(QWidget* parent, BinaryViewRef data) : QWidget(pare
 			});
 		connect(loadImageTable, &FilterableTableView::doubleClicked, this, [=](const QModelIndex& index)
 			{
-				auto name = loadImageModel->item(index.row(), 0)->text().toStdString();
+				auto name = proxyModel->data(index).toString().toStdString();
 				WorkerPriorityEnqueue([this, name]()
 					{
 						m_cache->LoadImageWithInstallName(name);
 					});
 			});
 
+		connect(loadImageTable, &FilterableTableView::filterTextChanged, this, [=](const QString& filter) {
+			proxyModel->setFilterFixedString(filter);
+			proxyModel->setFilterKeyColumn(-1);
+		});
+
 		auto loadImageLayout = new QVBoxLayout;
 		loadImageLayout->addWidget(loadImageFilterEdit);
 		loadImageLayout->addWidget(loadImageTable);
@@ -721,13 +740,17 @@ DSCTriageView::DSCTriageView(QWidget* parent, BinaryViewRef data) : QWidget(pare
 
 		m_bottomRegionTabs->addTab(loadImageWidget, "Load an Image");
 
-		loadImageTable->setModel(loadImageModel);
+		loadImageTable->setModel(proxyModel);
 
 		loadImageTable->horizontalHeader()->setSectionResizeMode(0, QHeaderView::Stretch);
 		loadImageTable->horizontalHeader()->setSectionResizeMode(1, QHeaderView::ResizeToContents);
 
 		loadImageTable->setSelectionBehavior(QAbstractItemView::SelectRows);
-		loadImageTable->setSelectionMode(QAbstractItemView::SingleSelection);
+		// loadImageTable->setSelectionMode(QAbstractItemView::SingleSelection);
+		loadImageTable->setSelectionMode(QAbstractItemView::MultiSelection);
+
+		loadImageTable->setSortingEnabled(true);
+
 
 		m_triageTabs->addTab(loadImageWidget, "Images");
 		defaultWidget = loadImageWidget;
@xusheng6 xusheng6 added the State: Awaiting Triage Issue is waiting for more in-depth triage from a developer label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Awaiting Triage Issue is waiting for more in-depth triage from a developer
Projects
None yet
Development

No branches or pull requests

3 participants