From b9a2e1026be2d256d29738900a26cc2aa26278e7 Mon Sep 17 00:00:00 2001
From: Vyacheslav Egorov <vegorov@google.com>
Date: Thu, 30 Aug 2018 12:20:19 +0000
Subject: [PATCH] [vm] Fix order of operations in Code::FinalizeCode

There should be no GCs between Instructions::New and
all write into the instructions object, because GC indiscriminately
write protects all code objects at the end of the GC.

In the previous version of the code assembler->GetCodeComments would
potentially trigger a GC and the code would crash trying to write
pointers into the code.

Also move FlushICache to the point after everything is written
into the code object.

Fixes #34030

Change-Id: I83742715f825f897a8612dda5ef7352c572517ee
Reviewed-on: https://dart-review.googlesource.com/72103
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
---
 runtime/vm/object.cc           | 53 +++++++++++++++++++---------------
 tests/language/language.status |  1 -
 2 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index ecdead7c760..46d58db5772 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -15632,32 +15632,17 @@ RawCode* Code::FinalizeCode(const char* name,
       compiler == nullptr ? 0 : compiler->UncheckedEntryOffset()));
   INC_STAT(Thread::Current(), total_instr_size, assembler->CodeSize());
   INC_STAT(Thread::Current(), total_code_size, assembler->CodeSize());
-
-  // Copy the instructions into the instruction area and apply all fixups.
-  // Embedded pointers are still in handles at this point.
-  MemoryRegion region(reinterpret_cast<void*>(instrs.PayloadStart()),
-                      instrs.Size());
-  assembler->FinalizeInstructions(region);
-  CPU::FlushICache(instrs.PayloadStart(), instrs.Size());
-
-#if defined(DART_PRECOMPILER)
-  if (stats != nullptr) {
-    stats->Finalize();
-    instrs.set_stats(stats);
-  }
-#endif
-
-  const Code::Comments& comments = assembler->GetCodeComments();
-
-  code.set_compile_timestamp(OS::GetCurrentMonotonicMicros());
-#ifndef PRODUCT
-  CodeCommentsWrapper comments_wrapper(comments);
-  CodeObservers::NotifyAll(name, instrs.PayloadStart(),
-                           assembler->prologue_offset(), instrs.Size(),
-                           optimized, &comments_wrapper);
-#endif
+  // Important: if GC is triggerred at any point between Instructions::New
+  // and here it would write protect instructions object that we are trying
+  // to fill in.
   {
     NoSafepointScope no_safepoint;
+    // Copy the instructions into the instruction area and apply all fixups.
+    // Embedded pointers are still in handles at this point.
+    MemoryRegion region(reinterpret_cast<void*>(instrs.PayloadStart()),
+                        instrs.Size());
+    assembler->FinalizeInstructions(region);
+
     const ZoneGrowableArray<intptr_t>& pointer_offsets =
         assembler->GetPointerOffsets();
     ASSERT(pointer_offsets.length() == pointer_offset_count);
@@ -15669,6 +15654,8 @@ RawCode* Code::FinalizeCode(const char* name,
       intptr_t offset_in_instrs = pointer_offsets[i];
       code.SetPointerOffsetAt(i, offset_in_instrs);
       uword addr = region.start() + offset_in_instrs;
+      ASSERT(instrs.PayloadStart() <= addr);
+      ASSERT((instrs.PayloadStart() + instrs.Size()) > addr);
       const Object* object = *reinterpret_cast<Object**>(addr);
       instrs.raw()->StorePointer(reinterpret_cast<RawObject**>(addr),
                                  object->raw());
@@ -15690,6 +15677,24 @@ RawCode* Code::FinalizeCode(const char* name,
                              instrs.raw()->Size(), VirtualMemory::kReadExecute);
     }
   }
+  CPU::FlushICache(instrs.PayloadStart(), instrs.Size());
+
+#if defined(DART_PRECOMPILER)
+  if (stats != nullptr) {
+    stats->Finalize();
+    instrs.set_stats(stats);
+  }
+#endif
+
+  const Code::Comments& comments = assembler->GetCodeComments();
+
+  code.set_compile_timestamp(OS::GetCurrentMonotonicMicros());
+#ifndef PRODUCT
+  CodeCommentsWrapper comments_wrapper(comments);
+  CodeObservers::NotifyAll(name, instrs.PayloadStart(),
+                           assembler->prologue_offset(), instrs.Size(),
+                           optimized, &comments_wrapper);
+#endif
   code.set_comments(comments);
   if (assembler->prologue_offset() >= 0) {
     code.SetPrologueOffset(assembler->prologue_offset());
diff --git a/tests/language/language.status b/tests/language/language.status
index b5fd39b0389..a655de210ae 100644
--- a/tests/language/language.status
+++ b/tests/language/language.status
@@ -330,7 +330,6 @@ closure_cycles_test: Pass, Slow
 large_class_declaration_test: SkipSlow # Uses too much memory.
 
 [ $arch == ia32 && $compiler == none && $runtime == vm && $system == windows ]
-disassemble_test: Pass, Crash # Issue 34030
 vm/optimized_stacktrace_test: Pass, Crash # Issue 28276
 
 [ $arch == ia32 && $mode == release && $runtime == vm ]