From b38c00a341dee90f1d2e49c73d455e4d167370a0 Mon Sep 17 00:00:00 2001
From: Kevin Millikin <kmillikin@google.com>
Date: Tue, 22 Aug 2017 09:27:21 +0200
Subject: [PATCH] Avoid triggering an ASSERT in TypedData

When a zero-sized string occurs as that last string in the Kernel
string table, the code would attempt to compute the address that lies
one element past the end of the string table's backing store.  Though
this is benign in C++ as long as that address is not dereferenced, it
would trigger an assertion failure in TypedData::DataAddr.

Avoid triggering the assertion by performing the address arithmetic in
the caller based on DataAddr(0) rather than relying on the address
arithmetic in DataAddr.

This fixes issue 30420.

BUG=https://github.com/dart-lang/sdk/issues/30420
R=aam@google.com, asiva@google.com

Review-Url: https://codereview.chromium.org/3003023002 .
---
 runtime/tests/vm/vm.status |  1 -
 runtime/vm/kernel_to_il.cc | 27 ++++++++++++++++++++-------
 runtime/vm/kernel_to_il.h  |  6 ++++++
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/runtime/tests/vm/vm.status b/runtime/tests/vm/vm.status
index 82f7b8a71a3..9b1b3710649 100644
--- a/runtime/tests/vm/vm.status
+++ b/runtime/tests/vm/vm.status
@@ -421,7 +421,6 @@ cc/Service_TokenStream: Fail
 cc/InjectNativeFields1: Crash
 cc/InjectNativeFields3: Crash
 cc/Service_TokenStream: Crash
-cc/IsolateReload_KernelIncrementalCompileAppAndLib: Crash # Issue 30420
 
 [ ($compiler == dartk) && ($runtime == vm) && ($system == macos) ]
 cc/IsolateReload_DanglingGetter_Class: Crash
diff --git a/runtime/vm/kernel_to_il.cc b/runtime/vm/kernel_to_il.cc
index 91a4f826569..a0272e9293d 100644
--- a/runtime/vm/kernel_to_il.cc
+++ b/runtime/vm/kernel_to_il.cc
@@ -136,13 +136,26 @@ uint8_t TranslationHelper::CharacterAt(StringIndex string_index,
   return string_data_.GetUint8(StringOffset(string_index) + index);
 }
 
+uint8_t* TranslationHelper::StringBuffer(StringIndex string_index) const {
+  // Though this implementation appears like it could be replaced by
+  // string_data_.DataAddr(StringOffset(string_index)), it can't quite.  If the
+  // last string in the string table is a zero length string, then the latter
+  // expression will try to return the address that is one past the backing
+  // store of the string_data_ table.  Though this is safe in C++ as long as the
+  // address is not dereferenced, it will trigger the assert in
+  // TypedData::DataAddr.
+  ASSERT(Thread::Current()->no_safepoint_scope_depth() > 0);
+  return reinterpret_cast<uint8_t*>(string_data_.DataAddr(0)) +
+         StringOffset(string_index);
+}
+
 bool TranslationHelper::StringEquals(StringIndex string_index,
                                      const char* other) {
-  NoSafepointScope no_safepoint;
   intptr_t length = strlen(other);
-  return (length == StringSize(string_index)) &&
-         (memcmp(string_data_.DataAddr(StringOffset(string_index)), other,
-                 length) == 0);
+  if (length != StringSize(string_index)) return false;
+
+  NoSafepointScope no_safepoint;
+  return memcmp(StringBuffer(string_index), other, length) == 0;
 }
 
 NameIndex TranslationHelper::CanonicalNameParent(NameIndex name) {
@@ -311,7 +324,7 @@ String& TranslationHelper::DartString(StringIndex string_index,
   uint8_t* buffer = Z->Alloc<uint8_t>(length);
   {
     NoSafepointScope no_safepoint;
-    memmove(buffer, string_data_.DataAddr(StringOffset(string_index)), length);
+    memmove(buffer, StringBuffer(string_index), length);
   }
   return String::ZoneHandle(Z, String::FromUTF8(buffer, length, space));
 }
@@ -331,7 +344,7 @@ String& TranslationHelper::DartSymbol(StringIndex string_index) const {
   uint8_t* buffer = Z->Alloc<uint8_t>(length);
   {
     NoSafepointScope no_safepoint;
-    memmove(buffer, string_data_.DataAddr(StringOffset(string_index)), length);
+    memmove(buffer, StringBuffer(string_index), length);
   }
   return String::ZoneHandle(Z, Symbols::FromUTF8(thread_, buffer, length));
 }
@@ -388,7 +401,7 @@ const String& TranslationHelper::DartSetterName(NameIndex parent,
   uint8_t* buffer = Z->Alloc<uint8_t>(size);
   {
     NoSafepointScope no_safepoint;
-    memmove(buffer, string_data_.DataAddr(StringOffset(setter)), size);
+    memmove(buffer, StringBuffer(setter), size);
   }
   String& name =
       String::ZoneHandle(Z, String::FromUTF8(buffer, size, allocation_space_));
diff --git a/runtime/vm/kernel_to_il.h b/runtime/vm/kernel_to_il.h
index e3920f1fc9d..88f23183526 100644
--- a/runtime/vm/kernel_to_il.h
+++ b/runtime/vm/kernel_to_il.h
@@ -284,6 +284,12 @@ class TranslationHelper {
 
   intptr_t StringOffset(StringIndex index) const;
   intptr_t StringSize(StringIndex index) const;
+
+  // The address of the backing store of the string with a given index.  If the
+  // backing store is in the VM's heap this address is not safe for GC (call the
+  // function and use the result within a NoSafepointScope).
+  uint8_t* StringBuffer(StringIndex index) const;
+
   uint8_t CharacterAt(StringIndex string_index, intptr_t index);
   bool StringEquals(StringIndex string_index, const char* other);