[ VM ] Ensure shared timeline objects are protected by locks

Timeline::Clear() could cause an user after free error if trying to allocate a timeline
event immediately after clearing the timeline buffer.

Timeline::Cleanup() could result in recorder_ being freed while timeline events are still
being processed.

Should fix https://github.com/flutter/flutter/issues/78076

TEST=tests/lib/developer/timeline_recorders_test.dart

Change-Id: Ia61b9c93986788c79ef6f2ff1907625b66fb4ea1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191802
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>

runtime/vm/timeline.cc

@@ -86,6 +86,8 @@ DEFINE_FLAG(charp,
 //
 // Locking notes:
 // The following locks are used by the timeline system:
+// - |Timeline::recorder_lock_| This lock is held whenever Timeline::recorder()
+// is accessed or modified.
 // - |TimelineEventRecorder::lock_| This lock is held whenever a
 // |TimelineEventBlock| is being requested or reclaimed.
 // - |Thread::timeline_block_lock_| This lock is held whenever a |Thread|'s
@@ -94,9 +96,10 @@ DEFINE_FLAG(charp,
 // |Thread|s.
 //
 // Locks must always be taken in the following order:
-//   |Thread::thread_list_lock_|
-//     |Thread::timeline_block_lock_|
-//       |TimelineEventRecorder::lock_|
+//   |Timeline::recorder_lock_|
+//     |Thread::thread_list_lock_|
+//       |Thread::timeline_block_lock_|
+//         |TimelineEventRecorder::lock_|
 //
 
 static TimelineEventRecorder* CreateTimelineRecorder() {
@@ -205,6 +208,10 @@ static bool HasStream(MallocGrowableArray<char*>* streams, const char* stream) {
 }
 
 void Timeline::Init() {
+  if (recorder_lock_ == nullptr) {
+    recorder_lock_ = new Mutex();
+  }
+  MutexLocker ml(recorder_lock_);
   ASSERT(recorder_ == NULL);
   recorder_ = CreateTimelineRecorder();
   ASSERT(recorder_ != NULL);
@@ -217,6 +224,8 @@ void Timeline::Init() {
 }
 
 void Timeline::Cleanup() {
+  ASSERT(recorder_lock_ != nullptr);
+  MutexLocker ml(recorder_lock_);
   ASSERT(recorder_ != NULL);
 
 #ifndef PRODUCT
@@ -243,6 +252,12 @@ TimelineEventRecorder* Timeline::recorder() {
 }
 
 void Timeline::ReclaimCachedBlocksFromThreads() {
+  ASSERT(recorder_lock_ != nullptr);
+  MutexLocker ml(recorder_lock_);
+  ReclaimCachedBlocksFromThreadsLocked();
+}
+
+void Timeline::ReclaimCachedBlocksFromThreadsLocked() {
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (recorder == NULL) {
     return;
@@ -274,6 +289,8 @@ void Timeline::PrintFlagsToJSONArray(JSONArray* arr) {
 }
 
 void Timeline::PrintFlagsToJSON(JSONStream* js) {
+  ASSERT(recorder_lock_ != nullptr);
+  MutexLocker ml(recorder_lock_);
   JSONObject obj(js);
   obj.AddProperty("type", "TimelineFlags");
   TimelineEventRecorder* recorder = Timeline::recorder();
@@ -301,11 +318,13 @@ void Timeline::PrintFlagsToJSON(JSONStream* js) {
 #endif
 
 void Timeline::Clear() {
+  ASSERT(recorder_lock_ != nullptr);
+  MutexLocker ml(recorder_lock_);
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (recorder == NULL) {
     return;
   }
-  ReclaimCachedBlocksFromThreads();
+  ReclaimCachedBlocksFromThreadsLocked();
   recorder->Clear();
 }
 
@@ -390,6 +409,7 @@ void TimelineEventArguments::Free() {
 
 TimelineEventRecorder* Timeline::recorder_ = NULL;
 MallocGrowableArray<char*>* Timeline::enabled_streams_ = NULL;
+Mutex* Timeline::recorder_lock_ = nullptr;
 
 #define TIMELINE_STREAM_DEFINE(name, fuchsia_name)                             \
   TimelineStream Timeline::stream_##name##_(#name, fuchsia_name, false);
@@ -556,6 +576,8 @@ void TimelineEvent::FormatArgument(intptr_t i,
 }
 
 void TimelineEvent::Complete() {
+  ASSERT(Timeline::recorder_lock() != nullptr);
+  MutexLocker ml(Timeline::recorder_lock());
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (recorder != NULL) {
     recorder->CompleteEvent(this);
@@ -775,6 +797,10 @@ TimelineStream::TimelineStream(const char* name,
 }
 
 TimelineEvent* TimelineStream::StartEvent() {
+  if (Timeline::recorder_lock() == nullptr) {
+    return nullptr;
+  }
+  MutexLocker ml(Timeline::recorder_lock());
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (!enabled() || (recorder == NULL)) {
     return NULL;
@@ -1137,6 +1163,7 @@ TimelineEventFixedBufferRecorder::TimelineEventFixedBufferRecorder(
 }
 
 TimelineEventFixedBufferRecorder::~TimelineEventFixedBufferRecorder() {
+  MutexLocker ml(&lock_);
   // Delete all blocks.
   for (intptr_t i = 0; i < num_blocks_; i++) {
     blocks_[i].Reset();
@@ -1334,6 +1361,7 @@ TimelineEventEndlessRecorder::TimelineEventEndlessRecorder()
     : head_(nullptr), tail_(nullptr), block_index_(0) {}
 
 TimelineEventEndlessRecorder::~TimelineEventEndlessRecorder() {
+  MutexLocker ml(&lock_);
   TimelineEventBlock* current = head_;
   head_ = tail_ = nullptr;
 
@@ -1424,6 +1452,9 @@ void TimelineEventEndlessRecorder::PrintJSONEvents(
 #endif
 
 void TimelineEventEndlessRecorder::Clear() {
+  OSThread* thread = OSThread::Current();
+  MutexLocker ml(thread->timeline_block_lock());
+  MutexLocker ml2(&lock_);
   TimelineEventBlock* current = head_;
   while (current != NULL) {
     TimelineEventBlock* next = current->next();
@@ -1431,8 +1462,8 @@ void TimelineEventEndlessRecorder::Clear() {
     current = next;
   }
   head_ = NULL;
+  tail_ = NULL;
   block_index_ = 0;
-  OSThread* thread = OSThread::Current();
   thread->set_timeline_block(NULL);
 }
 

runtime/vm/timeline.h

@@ -23,7 +23,7 @@
 #if defined(__MAC_10_14) || defined (__IPHONE_12_0)
 #define HOST_OS_SUPPORTS_SIGNPOST 1
 #endif
-//signpost.h exists in macOS 10.14, iOS 12 or above
+// signpost.h exists in macOS 10.14, iOS 12 or above
 #if defined(HOST_OS_SUPPORTS_SIGNPOST)
 #include <os/signpost.h>
 #else
@@ -123,15 +123,18 @@ class TimelineStream {
 
 class Timeline : public AllStatic {
  public:
-  // Initialize timeline system. Not thread safe.
+  // Initialize timeline system.
   static void Init();
 
-  // Cleanup timeline system. Not thread safe.
+  // Cleanup timeline system.
   static void Cleanup();
 
-  // Access the global recorder. Not thread safe.
+  // Access the global recorder. recorder_lock() must be held when accessing
+  // the returned |TimelineEventRecorder|.
   static TimelineEventRecorder* recorder();
 
+  static Mutex* recorder_lock() { return recorder_lock_; }
+
   // Reclaim all |TimelineEventBlocks|s that are cached by threads.
   static void ReclaimCachedBlocksFromThreads();
 
@@ -158,8 +161,13 @@ class Timeline : public AllStatic {
 #undef TIMELINE_STREAM_FLAGS
 
  private:
+  // Reclaims all |TimelineEventBlocks|s that are cached by threads, assuming
+  // that lock_ is held.
+  static void ReclaimCachedBlocksFromThreadsLocked();
+
   static TimelineEventRecorder* recorder_;
   static MallocGrowableArray<char*>* enabled_streams_;
+  static Mutex* recorder_lock_;
 
 #define TIMELINE_STREAM_DECLARE(name, fuchsia_name)                            \
   static TimelineStream stream_##name##_;