Skip to content

Commit

Permalink
Fix UAF in unittest.
Browse files Browse the repository at this point in the history
Discovered while reading the log in
https://luci-milo.appspot.com/ui/inv/build-8786629360992975185/test-results?q=DeviceCommandSetVolumeTest.CommandTimeout&sortby=&groupby=
while running MSAN tests on an unrelated patch. The critical bit is:

==1719523==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x564366cfa43c in find_if<std::Cr::__wrap_iter<base::internal::UncheckedObserverAdapter *>, (lambda at ../../base/ranges/algorithm.h:31:10)> ./../../buildtools/third_party/libc++/trunk/include/__algorithm/find_if.h:24:3
    #1 0x564366cfa43c in find_if<std::Cr::__wrap_iter<base::internal::UncheckedObserverAdapter *>, (lambda at ../../base/observer_list.h:288:21), base::identity, std::Cr::random_access_iterator_tag> ./../../base/ranges/algorithm.h:465:10
    #2 0x564366cfa43c in find_if<std::Cr::vector<base::internal::UncheckedObserverAdapter, std::Cr::allocator<base::internal::UncheckedObserverAdapter> > &, (lambda at ../../base/observer_list.h:288:21), base::identity, std::Cr::random_access_iterator_tag> ./../../base/ranges/algorithm.h:483:10
    #3 0x564366cfa43c in base::ObserverList<ash::CrasAudioHandler::AudioObserver, false, true, base::internal::UncheckedObserverAdapter>::RemoveObserver(ash::CrasAudioHandler::AudioObserver const*) ./../../base/observer_list.h:287:21
    #4 0x56434c48ca96 in RemoveObserver ./../../chromeos/ash/components/audio/cras_audio_handler.h:924:13
    #5 0x56434c48ca96 in Reset ./../../base/scoped_observation.h:115:7
    #6 0x56434c48ca96 in ~ScopedObservation ./../../base/scoped_observation.h:101:26
    #7 0x56434c48ca96 in policy::(anonymous namespace)::TestAudioObserver::~TestAudioObserver() ./../../chrome/browser/ash/policy/remote_commands/device_command_set_volume_job_unittest.cc:64:41
    #8 0x56434c48c6f6 in ~DeviceCommandSetVolumeTest ./../../chrome/browser/ash/policy/remote_commands/device_command_set_volume_job_unittest.cc:88:7
    #9 0x56434c48c6f6 in policy::DeviceCommandSetVolumeTest_CommandTimeout_Test::~DeviceCommandSetVolumeTest_CommandTimeout_Test() ./../../chrome/browser/ash/policy/remote_commands/device_command_set_volume_job_unittest.cc:165:1
    #10 0x56434fd5a2ec in DeleteSelf_ ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:318:24
    #11 0x56434fd5a2ec in HandleExceptionsInMethodIfSupported<testing::Test, void> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #12 0x56434fd5a2ec in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2855:5
    ...
  Uninitialized value was created by a heap deallocation
    #0 0x5643395c3b89 in operator delete(void*) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/msan/msan_new_delete.cpp:74:44
    #1 0x564366cf54d8 in ash::CrasAudioHandler::Shutdown() ./../../chromeos/ash/components/audio/cras_audio_handler.cc:162:3
    #2 0x564366651d92 in ash::AshTestHelper::TearDown() ./../../ash/test/ash_test_helper.cc:180:5
    #3 0x56436664beb4 in ash::AshTestBase::TearDown() ./../../ash/test/ash_test_base.cc:165:21
    #4 0x56434fd5a166 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2849:11
    ...

Looking at the test, it observes the global CrasAudioHandler, but never
stops observing it, and that handler is destroyed before the test
object's destructor runs (which in turn destroys the ScopedObservation).

Fix by explicitly stopping observing the handler before it's torn down.

Bug: none
Change-Id: I4c223380cb1bec87a4a5725452e35f4004013a72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335271
Reviewed-by: Alexander Hendrich <[email protected]>
Commit-Queue: Alexander Hendrich <[email protected]>
Auto-Submit: Peter Kasting <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1117177}
  • Loading branch information
pkasting authored and Chromium LUCI CQ committed Mar 14, 2023
1 parent 9454a41 commit 4edf381
Showing 1 changed file with 11 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ class TestAudioObserver : public ash::CrasAudioHandler::AudioObserver {
observation_.Observe(&handler);
}

void Shutdown() { observation_.Reset(); }

// `ash::CrasAudioHandler::AudioObserver` implementation:
void OnOutputNodeVolumeChanged(uint64_t node_id, int volume) override {
waiter_.SetValue();
Expand Down Expand Up @@ -96,6 +98,7 @@ class DeviceCommandSetVolumeTest : public ChromeAshTestBase {

// testing::Test
void SetUp() override;
void TearDown() override;

void RunJob(RemoteCommandJob& job) {
base::test::TestFuture<void> job_finished_future;
Expand All @@ -122,6 +125,14 @@ void DeviceCommandSetVolumeTest::SetUp() {
audio_observer_.Initialize(*ash::CrasAudioHandler::Get());
}

void DeviceCommandSetVolumeTest::TearDown() {
// The TearDown() call below may destroy the CrasAudioHandler being observed,
// so stop observing it before that happens.
audio_observer_.Shutdown();

ChromeAshTestBase::TearDown();
}

void VerifyResults(const RemoteCommandJob& job,
int expected_volume,
bool expected_muted) {
Expand Down

0 comments on commit 4edf381

Please sign in to comment.