From cf74c2b50c401ad099765c9c4e1c7b8da62da482 Mon Sep 17 00:00:00 2001 From: sbc Date: Tue, 2 Jun 2015 10:19:35 -0700 Subject: [PATCH] [NaCl SDK] nacl_io: Fix data race in fake_var_mananger Thanks msan! CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_nacl_sdk;tryserver.chromium.mac:mac_nacl_sdk;tryserver.chromium.win:win_nacl_sdk Review URL: https://codereview.chromium.org/1160673002 Cr-Commit-Position: refs/heads/master@{#332417} --- .../nacl_io_test/fake_ppapi/fake_var_manager.cc | 26 ++++++++++++++++------ .../nacl_io_test/fake_ppapi/fake_var_manager.h | 6 ++++- .../src/tests/nacl_io_test/kernel_wrap_test.cc | 8 +++++++ 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.cc b/native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.cc index a8f3a2a228c6..bddedd25d03b 100644 --- a/native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.cc +++ b/native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.cc @@ -3,8 +3,8 @@ // found in the LICENSE file. #include "fake_ppapi/fake_var_manager.h" - #include "gtest/gtest.h" +#include "sdk_util/auto_lock.h" FakeVarManager::FakeVarManager() : debug(false), next_id_(1) {} @@ -19,6 +19,7 @@ FakeVarManager::~FakeVarManager() { } FakeVarData* FakeVarManager::CreateVarData() { + AUTO_LOCK(lock_); Id id = next_id_++; FakeVarData data; data.id = id; @@ -28,12 +29,13 @@ FakeVarData* FakeVarManager::CreateVarData() { } void FakeVarManager::AddRef(PP_Var var) { + AUTO_LOCK(lock_); // From ppb_var.h: // AddRef() adds a reference to the given var. If this is not a refcounted // object, this function will do nothing so you can always call it no matter // what the type. - FakeVarData* var_data = GetVarData(var); + FakeVarData* var_data = GetVarData_Locked(var); if (!var_data) return; @@ -73,7 +75,7 @@ std::string FakeVarManager::Describe(const FakeVarData& var_data) { return rtn.str(); } -void FakeVarManager::DestroyVarData(FakeVarData* var_data) { +void FakeVarManager::DestroyVarData_Locked(FakeVarData* var_data) { // Release each PP_Var in the array switch (var_data->type) { @@ -81,7 +83,7 @@ void FakeVarManager::DestroyVarData(FakeVarData* var_data) { FakeArrayType& vector = var_data->array_value; for (FakeArrayType::iterator it = vector.begin(); it != vector.end(); ++it) { - Release(*it); + Release_Locked(*it); } vector.clear(); break; @@ -96,7 +98,7 @@ void FakeVarManager::DestroyVarData(FakeVarData* var_data) { FakeDictType& dict = var_data->dict_value; for (FakeDictType::iterator it = dict.begin(); it != dict.end(); it++) { - Release(it->second); + Release_Locked(it->second); } dict.clear(); break; @@ -107,6 +109,11 @@ void FakeVarManager::DestroyVarData(FakeVarData* var_data) { } FakeVarData* FakeVarManager::GetVarData(PP_Var var) { + AUTO_LOCK(lock_); + return GetVarData_Locked(var); +} + +FakeVarData* FakeVarManager::GetVarData_Locked(PP_Var var) { switch (var.type) { // These types don't have any var data as their data // is stored directly in the var's value union. @@ -130,12 +137,17 @@ FakeVarData* FakeVarManager::GetVarData(PP_Var var) { } void FakeVarManager::Release(PP_Var var) { + AUTO_LOCK(lock_); + Release_Locked(var); +} + +void FakeVarManager::Release_Locked(PP_Var var) { // From ppb_var.h: // Release() removes a reference to given var, deleting it if the internal // reference count becomes 0. If the given var is not a refcounted object, // this function will do nothing so you can always call it no matter what // the type. - FakeVarData* var_data = GetVarData(var); + FakeVarData* var_data = GetVarData_Locked(var); if (!var_data) { if (debug) printf("Releasing simple var\n"); @@ -152,5 +164,5 @@ void FakeVarManager::Release(PP_Var var) { var_data->ref_count); if (var_data->ref_count == 0) - DestroyVarData(var_data); + DestroyVarData_Locked(var_data); } diff --git a/native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.h b/native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.h index 8f02bb5cf263..5c7909f876cf 100644 --- a/native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.h +++ b/native_client_sdk/src/tests/nacl_io_test/fake_ppapi/fake_var_manager.h @@ -11,6 +11,7 @@ #include #include "sdk_util/macros.h" +#include "sdk_util/simple_lock.h" typedef std::vector FakeArrayType; typedef std::map FakeDictType; @@ -41,7 +42,9 @@ class FakeVarManager { bool debug; private: - void DestroyVarData(FakeVarData* var); + void Release_Locked(PP_Var var); + FakeVarData* GetVarData_Locked(PP_Var var); + void DestroyVarData_Locked(FakeVarData* var); typedef uint64_t Id; typedef std::map VarMap; @@ -49,6 +52,7 @@ class FakeVarManager { Id next_id_; VarMap var_map_; + sdk_util::SimpleLock lock_; DISALLOW_COPY_AND_ASSIGN(FakeVarManager); }; diff --git a/native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc b/native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc index efb30fbe6b2b..75ec6b72947e 100644 --- a/native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc +++ b/native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc @@ -159,6 +159,14 @@ class KernelWrapTest : public ::testing::Test { ON_CALL(mock, write(_, _, _)) .WillByDefault(Invoke(this, &KernelWrapTest::DefaultWrite)); EXPECT_CALL(mock, write(_, _, _)).Times(AnyNumber()); + + // Ignore calls to munmap. These can be generated from within the standard + // library malloc implementation so can be expected at pretty much any time. + // Returning zero is fine since the real munmap see also run. + // See kernel_wrap_newlib.cc. + ON_CALL(mock, munmap(_, _)) + .WillByDefault(Return(0)); + EXPECT_CALL(mock, munmap(_, _)).Times(AnyNumber()); } void TearDown() { -- 2.11.4.GIT