From b906e10a5d3681c1a23008f85ff01ee30086d439 Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Tue, 25 Jun 2013 17:37:10 -0400 Subject: [PATCH] Fix crash in InspectorWindow. - In the case where retrieval of a memory block failed, InspectorWindow didn't handle the notification. Consequently, it never removed itself as a listener from the failed block, nor did it release its reference for it. Consequently, if one attempted to retrieve data from the same block again, walking the listener list would crash due to the already-deleted entry in the list. - The success case had the same problem with regards to not removing its listener, but was masked by virtue of the inspector currently being the only user of the memory block manager, so in the latter case the blocks would be properly released/destroyed and the aforementioned walk would never occur. - Adjust locking a bit to ensure that manipulating the listener list always happens with the team lock held. - Style fixes. --- .../debugger/controllers/TeamDebugger.cpp | 9 ++- .../gui/inspector_window/InspectorWindow.cpp | 79 ++++++++++++++----- .../gui/inspector_window/InspectorWindow.h | 2 + 3 files changed, 68 insertions(+), 22 deletions(-) diff --git a/src/apps/debugger/controllers/TeamDebugger.cpp b/src/apps/debugger/controllers/TeamDebugger.cpp index 7974368833..6026df67d5 100644 --- a/src/apps/debugger/controllers/TeamDebugger.cpp +++ b/src/apps/debugger/controllers/TeamDebugger.cpp @@ -1690,12 +1690,12 @@ TeamDebugger::_HandleInspectAddress(target_addr_t address, return; } - if (!memoryBlock->HasListener(listener)) - memoryBlock->AddListener(listener); - if (!memoryBlock->IsValid()) { AutoLocker< ::Team> teamLocker(fTeam); + if (!memoryBlock->HasListener(listener)) + memoryBlock->AddListener(listener); + TeamMemory* memory = fTeam->GetTeamMemory(); // schedule the job status_t result; @@ -1703,7 +1703,10 @@ TeamDebugger::_HandleInspectAddress(target_addr_t address, new(std::nothrow) RetrieveMemoryBlockJob(fTeam, memory, memoryBlock), this)) != B_OK) { + + memoryBlock->NotifyDataRetrieved(result); memoryBlock->ReleaseReference(); + _NotifyUser("Inspect Address", "Failed to retrieve memory data: %s", strerror(result)); } diff --git a/src/apps/debugger/user_interface/gui/inspector_window/InspectorWindow.cpp b/src/apps/debugger/user_interface/gui/inspector_window/InspectorWindow.cpp index 1293e2f6ad..333fa3065c 100644 --- a/src/apps/debugger/user_interface/gui/inspector_window/InspectorWindow.cpp +++ b/src/apps/debugger/user_interface/gui/inspector_window/InspectorWindow.cpp @@ -27,8 +27,9 @@ enum { - MSG_NAVIGATE_PREVIOUS_BLOCK = 'npbl', - MSG_NAVIGATE_NEXT_BLOCK = 'npnl' + MSG_NAVIGATE_PREVIOUS_BLOCK = 'npbl', + MSG_NAVIGATE_NEXT_BLOCK = 'npnl', + MSG_MEMORY_BLOCK_RETRIEVED = 'mbre', }; @@ -52,8 +53,7 @@ InspectorWindow::InspectorWindow(::Team* team, UserInterfaceListener* listener, InspectorWindow::~InspectorWindow() { - if (fCurrentBlock != NULL) - { + if (fCurrentBlock != NULL) { fCurrentBlock->RemoveListener(this); fCurrentBlock->ReleaseReference(); } @@ -186,15 +186,14 @@ InspectorWindow::_Init() void -InspectorWindow::MessageReceived(BMessage* msg) +InspectorWindow::MessageReceived(BMessage* message) { - switch (msg->what) { + switch (message->what) { case MSG_INSPECT_ADDRESS: { target_addr_t address = 0; bool addressValid = false; - if (msg->FindUInt64("address", &address) != B_OK) - { + if (message->FindUInt64("address", &address) != B_OK) { ExpressionParser parser; parser.SetSupportHexInput(true); const char* addressExpression = fAddressInput->Text(); @@ -238,10 +237,9 @@ InspectorWindow::MessageReceived(BMessage* msg) case MSG_NAVIGATE_PREVIOUS_BLOCK: case MSG_NAVIGATE_NEXT_BLOCK: { - if (fCurrentBlock != NULL) - { + if (fCurrentBlock != NULL) { target_addr_t address = fCurrentBlock->BaseAddress(); - if (msg->what == MSG_NAVIGATE_PREVIOUS_BLOCK) + if (message->what == MSG_NAVIGATE_PREVIOUS_BLOCK) address -= fCurrentBlock->Size(); else address += fCurrentBlock->Size(); @@ -252,9 +250,44 @@ InspectorWindow::MessageReceived(BMessage* msg) } break; } + case MSG_MEMORY_BLOCK_RETRIEVED: + { + TeamMemoryBlock* block = NULL; + status_t result; + if (message->FindPointer("block", + reinterpret_cast(&block)) != B_OK + || message->FindInt32("result", &result) != B_OK) { + break; + } + + { + AutoLocker< ::Team>(fTeam); + block->RemoveListener(this); + } + + if (result == B_OK) { + fCurrentBlock = block; + fMemoryView->SetTargetAddress(block, fCurrentAddress); + fPreviousBlockButton->SetEnabled(true); + fNextBlockButton->SetEnabled(true); + } else { + BString errorMessage; + errorMessage.SetToFormat("Unable to read address 0x%" B_PRIx64 + ": %s", block->BaseAddress(), strerror(result)); + + BAlert* alert = new(std::nothrow) BAlert("Inspect address", + errorMessage.String(), "Close"); + if (alert == NULL) + break; + + alert->Go(NULL); + block->ReleaseReference(); + } + break; + } default: { - BWindow::MessageReceived(msg); + BWindow::MessageReceived(message); break; } } @@ -275,13 +308,21 @@ InspectorWindow::QuitRequested() void InspectorWindow::MemoryBlockRetrieved(TeamMemoryBlock* block) { - AutoLocker lock(this); - if (lock.IsLocked()) { - fCurrentBlock = block; - fMemoryView->SetTargetAddress(block, fCurrentAddress); - fPreviousBlockButton->SetEnabled(true); - fNextBlockButton->SetEnabled(true); - } + BMessage message(MSG_MEMORY_BLOCK_RETRIEVED); + message.AddPointer("block", block); + message.AddInt32("result", B_OK); + PostMessage(&message); +} + + +void +InspectorWindow::MemoryBlockRetrievalFailed(TeamMemoryBlock* block, + status_t result) +{ + BMessage message(MSG_MEMORY_BLOCK_RETRIEVED); + message.AddPointer("block", block); + message.AddInt32("result", result); + PostMessage(&message); } diff --git a/src/apps/debugger/user_interface/gui/inspector_window/InspectorWindow.h b/src/apps/debugger/user_interface/gui/inspector_window/InspectorWindow.h index 320e344d9f..528701d9b8 100644 --- a/src/apps/debugger/user_interface/gui/inspector_window/InspectorWindow.h +++ b/src/apps/debugger/user_interface/gui/inspector_window/InspectorWindow.h @@ -41,6 +41,8 @@ public: // TeamMemoryBlock::Listener virtual void MemoryBlockRetrieved(TeamMemoryBlock* block); + virtual void MemoryBlockRetrievalFailed( + TeamMemoryBlock* block, status_t result); // MemoryView::Listener virtual void TargetAddressChanged(target_addr_t address);