Resolution for issue #28746: changed order in which locks/flags are grabbed in MallocHooks to preve…

runtime/vm/malloc_hooks.cc

 // Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "platform/globals.h"
 
 #if defined(DART_USE_TCMALLOC) && !defined(PRODUCT)
 
 #include "vm/malloc_hooks.h"
 
 #include "gperftools/malloc_hook.h"
 
 #include "platform/assert.h"
 #include "vm/hash_map.h"
 #include "vm/json_stream.h"
 #include "vm/lockers.h"
 
 namespace dart {
 
 // A locker-type class to automatically grab and release the
 // in_malloc_hook_flag_.
 class MallocHookScope {
  public:
   static void InitMallocHookFlag() {
+    MutexLocker ml(malloc_hook_scope_mutex_);
     ASSERT(in_malloc_hook_flag_ == kUnsetThreadLocalKey);
     in_malloc_hook_flag_ = OSThread::CreateThreadLocal();
     OSThread::SetThreadLocal(in_malloc_hook_flag_, 0);
   }
 
   static void DestroyMallocHookFlag() {
+    MutexLocker ml(malloc_hook_scope_mutex_);
     ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey);
     OSThread::DeleteThreadLocal(in_malloc_hook_flag_);
     in_malloc_hook_flag_ = kUnsetThreadLocalKey;
   }
 
   MallocHookScope() {
+    MutexLocker ml(malloc_hook_scope_mutex_);
     ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey);
     OSThread::SetThreadLocal(in_malloc_hook_flag_, 1);
   }
 
   ~MallocHookScope() {
+    MutexLocker ml(malloc_hook_scope_mutex_);
     ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey);
     OSThread::SetThreadLocal(in_malloc_hook_flag_, 0);
   }
 
   static bool IsInHook() {
-    ASSERT(in_malloc_hook_flag_ != kUnsetThreadLocalKey);
+    MutexLocker ml(malloc_hook_scope_mutex_);

[bkonyi, 2017/02/17 21:15:11]

I added a mutex to the MallocHookScope class so that the flag can't be destroyed
while we're checking it or trying to set it.

+    if (in_malloc_hook_flag_ == kUnsetThreadLocalKey) {
+      // Bail out if the malloc hook flag is invalid. This means that
+      // MallocHookState::TearDown() has been called and MallocHookScope is no
+      // longer intitialized. Don't worry if MallocHookState::TearDown() is
+      // called before the hooks grab the mutex, since
+      // MallocHookScope::Active() is checked after the lock is taken before
+      // proceeding to act on the allocation/free.
+      return false;
+    }
     return OSThread::GetThreadLocal(in_malloc_hook_flag_);
   }
 
+  static bool Active() {

[zra, 2017/02/17 21:42:26]

rm

+    MutexLocker ml(malloc_hook_scope_mutex_);
+    return in_malloc_hook_flag_ != kUnsetThreadLocalKey;

[bkonyi, 2017/02/17 21:15:11]

I'm guessing this needs parens around the boolean expression, right?

+  }
+
  private:
+  static Mutex* malloc_hook_scope_mutex_;
   static ThreadLocalKey in_malloc_hook_flag_;
 
   DISALLOW_ALLOCATION();
   DISALLOW_COPY_AND_ASSIGN(MallocHookScope);
 };
 
 
 // Custom key/value trait specifically for address/size pairs. Unlike
 // RawPointerKeyValueTrait, the default value is -1 as 0 can be a valid entry.
 class AddressKeyValueTrait {
@@ skipping 104 matching lines @@
   static intptr_t original_pid_;
   static Mutex* malloc_hook_mutex_;
   static intptr_t allocation_count_;
   static intptr_t heap_allocated_memory_in_bytes_;
   static AddressMap* address_map_;
 
   static const intptr_t kInvalidPid = -1;
 };
 
 
+// MallocHookScope state.
+Mutex* MallocHookScope::malloc_hook_scope_mutex_ = new Mutex();
+ThreadLocalKey MallocHookScope::in_malloc_hook_flag_ = kUnsetThreadLocalKey;
+
 // MallocHooks state / locks.
-ThreadLocalKey MallocHookScope::in_malloc_hook_flag_ = kUnsetThreadLocalKey;
 bool MallocHooksState::active_ = false;
 intptr_t MallocHooksState::original_pid_ = MallocHooksState::kInvalidPid;
 Mutex* MallocHooksState::malloc_hook_mutex_ = new Mutex();
 
 // Memory allocation state information.
 intptr_t MallocHooksState::allocation_count_ = 0;
 intptr_t MallocHooksState::heap_allocated_memory_in_bytes_ = 0;
 AddressMap* MallocHooksState::address_map_ = NULL;
 
 
@@ skipping 17 matching lines @@
   MutexLocker ml(MallocHooksState::malloc_hook_mutex());
   ASSERT(MallocHooksState::Active());
 
   // Remove malloc hooks.
   bool success = false;
   success = MallocHook::RemoveNewHook(&MallocHooksState::RecordAllocHook);
   ASSERT(success);
   success = MallocHook::RemoveDeleteHook(&MallocHooksState::RecordFreeHook);
   ASSERT(success);
 
   MallocHooksState::TearDown();

[zra, 2017/02/17 21:29:53]

 !MallocHooksState::Active() implies !MallocHookScope::Active(), so there is no
need to check both conditions below.

   MallocHookScope::DestroyMallocHookFlag();
 }
 
 
 void MallocHooks::ResetStats() {
   MutexLocker ml(MallocHooksState::malloc_hook_mutex());
   if (MallocHooksState::Active()) {
     MallocHooksState::ResetStats();
   }
 }
@@ skipping 37 matching lines @@
   MutexLocker ml(MallocHooksState::malloc_hook_mutex());
   return MallocHooksState::heap_allocated_memory_in_bytes();
 }
 
 
 void MallocHooksState::RecordAllocHook(const void* ptr, size_t size) {
   if (MallocHookScope::IsInHook() || !MallocHooksState::IsOriginalProcess()) {
     return;
   }
 
-  // Set the malloc hook flag before grabbing the mutex to avoid calling hooks
-  // again.
-  MallocHookScope mhs;
   MutexLocker ml(MallocHooksState::malloc_hook_mutex());
-  if ((ptr != NULL) && MallocHooksState::Active()) {
+  // Now that we hold the lock, check to make sure everything is still active.
+  if ((ptr != NULL) && MallocHooksState::Active() &&
+      MallocHookScope::Active()) {

[bkonyi, 2017/02/17 21:15:11]

This extra condition, MallocHookScope::Active(), is just to ensure that the
MallocHookScope flag wasn't destroyed since the last time we checked it. 

[zra, 2017/02/17 21:29:53]

I don't think it's possible that we could have MallocHooksState::Active() and
!MallocHookScope::Active(). See above.

+    // Set the malloc hook flag to avoid calling hooks again if memory is
+    // allocated/freed below.
+    MallocHookScope mhs;
     MallocHooksState::IncrementHeapAllocatedMemoryInBytes(size);
     MallocHooksState::address_map()->Insert(ptr, size);
   }
 }
 
 
 void MallocHooksState::RecordFreeHook(const void* ptr) {
   if (MallocHookScope::IsInHook() || !MallocHooksState::IsOriginalProcess()) {
     return;
   }
 
-  // Set the malloc hook flag before grabbing the mutex to avoid calling hooks
-  // again.
-  MallocHookScope mhs;
   MutexLocker ml(MallocHooksState::malloc_hook_mutex());
-  if ((ptr != NULL) && MallocHooksState::Active()) {
+  // Now that we hold the lock, check to make sure everything is still active.
+  if ((ptr != NULL) && MallocHooksState::Active() &&
+      MallocHookScope::Active()) {

[zra, 2017/02/17 21:29:53]

ditto

+    // Set the malloc hook flag to avoid calling hooks again if memory is
+    // allocated/freed below.
+    MallocHookScope mhs;
     intptr_t size = 0;
     if (MallocHooksState::address_map()->Lookup(ptr, &size)) {
       MallocHooksState::DecrementHeapAllocatedMemoryInBytes(size);
       MallocHooksState::address_map()->Remove(ptr);
     }
   }
 }
 
 }  // namespace dart
 
 #endif  // defined(DART_USE_TCMALLOC) && !defined(PRODUCT)