[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}

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 a8f3a2a..bddedd2 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 8f02bb5..5c7909f 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 <vector>
 
 #include "sdk_util/macros.h"
+#include "sdk_util/simple_lock.h"
 
 typedef std::vector<PP_Var> FakeArrayType;
 typedef std::map<std::string, PP_Var> 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<Id, FakeVarData> 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 efb30fbe..75ec6b7 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() {