[vm/service] Fix JSON array parsing bug that causes seg fault

Changed the definition of `end` to point 1 past the end of the element,
rather than the end of the element, which avoids the need for buggy
look-ahead style lookups.

I think this must have been a long standing bug, and we just never sent
empty JSON arrays to the service until now, because I didn't change this
logic when I refactored it.

Bug: #53990
Fixes: #53990
Change-Id: I89ece7a036d0b71610a153e708f40aeabab5367c
TEST=Added Service_ParseJSONArray
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/335980
Commit-Queue: Liam Appelbe <liama@google.com>
Auto-Submit: Liam Appelbe <liama@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>

runtime/vm/service.cc

@@ -3643,26 +3643,32 @@ static intptr_t ParseJSONCollection(Thread* thread,
   intptr_t n = strlen(str);
   if (n < 2) {
     return -1;
+  } else if (n == 2) {
+    return 0;
   }
+  // The JSON string array looks like [abc, def]. There are no quotes around the
+  // strings, but there is a space after the comma. start points to the first
+  // character of the element. end points to the separator after the element
+  // (']' or ',').
   intptr_t start = 1;
   while (start < n) {
     intptr_t end = start;
-    while ((str[end + 1] != ',') && (str[end + 1] != ']')) {
-      end++;
-    }
-    if (end == start) {
-      // Empty element
-      break;
+    while (end < n) {
+      const char c = str[end];
+      if (c == ',' || c == ']') {
+        break;
+      }
+      ++end;
     }
-    add(&str[start], end - start + 1);
-    start = end + 3;
+    add(&str[start], end - start);
+    start = end + 2;
   }
   return 0;
 }
 
-static intptr_t ParseJSONArray(Thread* thread,
-                               const char* str,
-                               const GrowableObjectArray& elements) {
+intptr_t ParseJSONArray(Thread* thread,
+                        const char* str,
+                        const GrowableObjectArray& elements) {
   Zone* zone = thread->zone();
   return ParseJSONCollection(
       thread, str, [zone, &elements](const char* start, intptr_t length) {

runtime/vm/service.h

@@ -13,6 +13,7 @@
 #include "vm/object_id_ring.h"
 #include "vm/os_thread.h"
 #include "vm/tagged_pointer.h"
+#include "vm/thread.h"
 
 namespace dart {
 
@@ -269,6 +270,11 @@ class Service : public AllStatic {
   static intptr_t dart_library_kernel_len_;
 };
 
+// Visible for testing.
+intptr_t ParseJSONArray(Thread* thread,
+                        const char* str,
+                        const GrowableObjectArray& elements);
+
 }  // namespace dart
 
 #endif  // RUNTIME_VM_SERVICE_H_

runtime/vm/service_test.cc

@@ -726,6 +726,72 @@ ISOLATE_UNIT_TEST_CASE(Service_Profile) {
 
 #endif  // !defined(TARGET_ARCH_ARM64)
 
+ISOLATE_UNIT_TEST_CASE(Service_ParseJSONArray) {
+  {
+    const auto& elements =
+        GrowableObjectArray::Handle(GrowableObjectArray::New());
+    EXPECT_EQ(-1, ParseJSONArray(thread, "", elements));
+    EXPECT_EQ(-1, ParseJSONArray(thread, "[", elements));
+  }
+
+  {
+    const auto& elements =
+        GrowableObjectArray::Handle(GrowableObjectArray::New());
+    EXPECT_EQ(0, ParseJSONArray(thread, "[]", elements));
+    EXPECT_EQ(0, elements.Length());
+  }
+
+  {
+    const auto& elements =
+        GrowableObjectArray::Handle(GrowableObjectArray::New());
+    EXPECT_EQ(0, ParseJSONArray(thread, "[a]", elements));
+    EXPECT_EQ(1, elements.Length());
+    auto& element = String::Handle();
+    element ^= elements.At(0);
+    EXPECT(element.Equals("a"));
+  }
+
+  {
+    const auto& elements =
+        GrowableObjectArray::Handle(GrowableObjectArray::New());
+    EXPECT_EQ(0, ParseJSONArray(thread, "[abc, def]", elements));
+    EXPECT_EQ(2, elements.Length());
+    auto& element = String::Handle();
+    element ^= elements.At(0);
+    EXPECT(element.Equals("abc"));
+    element ^= elements.At(1);
+    EXPECT(element.Equals("def"));
+  }
+
+  {
+    const auto& elements =
+        GrowableObjectArray::Handle(GrowableObjectArray::New());
+    EXPECT_EQ(0, ParseJSONArray(thread, "[abc, def, ghi]", elements));
+    EXPECT_EQ(3, elements.Length());
+    auto& element = String::Handle();
+    element ^= elements.At(0);
+    EXPECT(element.Equals("abc"));
+    element ^= elements.At(1);
+    EXPECT(element.Equals("def"));
+    element ^= elements.At(2);
+    EXPECT(element.Equals("ghi"));
+  }
+
+  {
+    const auto& elements =
+        GrowableObjectArray::Handle(GrowableObjectArray::New());
+    EXPECT_EQ(0, ParseJSONArray(thread, "[abc, , ghi]", elements));
+    EXPECT_EQ(3, elements.Length());
+    auto& element = String::Handle();
+    element ^= elements.At(0);
+    EXPECT(element.Equals("abc"));
+    element ^= elements.At(1);
+    EXPECT(element.Equals(""));
+    element ^= elements.At(2);
+    EXPECT(element.Equals("ghi"));
+  }
+}
+
 #endif  // !PRODUCT
 
 }  // namespace dart