Fix release mode crash in String::DecodeURI.

runtime/vm/object.cc

Index: runtime/vm/object.cc
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index 9b7924075cb642f4ab24c5a010a9cac9ec1a2a83..7c817e13af4005381536f2e88fe53628362d766c 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -14462,6 +14462,17 @@ static bool IsPercent(int32_t c) {
 }
 
 
+static bool IsHexCharacter(int32_t c) {
+  if (c >= '0' && c <= '9') {
+    return true;
+  }
+  if (c >= 'A' && c <= 'F') {
+    return true;
+  }
+  return false;
+}
+
+
 static bool IsURISafeCharacter(int32_t c) {
   if ((c >= '0') && (c <= '9')) {
     return true;
@@ -14545,15 +14556,39 @@ RawString* String::DecodeURI(const String& str) {
   CodePointIterator cpi(str);
   intptr_t num_escapes = 0;
   intptr_t len = str.Length();
+  bool valid = true;
   {
     CodePointIterator cpi(str);
-    while (cpi.Next()) {
+    while (valid && cpi.Next()) {
       int32_t code_point = cpi.Current();
       if (IsPercent(code_point)) {
+        // Verify that the two characters following the % are hex digits.
+        if (!cpi.Next()) {
+          valid = false;
+          break;
+        }
+        int32_t code_point = cpi.Current();
+        if (!IsHexCharacter(code_point)) {
+          valid = false;
+          break;
+        }
+        if (!cpi.Next()) {
+          valid = false;
+          break;
+        }
+        code_point = cpi.Current();
+        if (!IsHexCharacter(code_point)) {
+          valid = false;
+          break;
+        }
         num_escapes += 2;
       }
     }
   }
+  if (!valid) {
+    // Invalid, return original string.
+    return str.raw();
+  }

[siva, 2013/12/21 00:10:36]

You seem to always break out of the while loop if an invalid character is seen
and then check and return original string on errors.

Why not just replace all breaks with "return str.raw()" and
get rid of the local variable 'valid'.

[turnidge, 2014/01/06 20:34:47]

Agree with Siva that the code could be simpler.

I think the code is a bit more readable if we combine all of these tests
together:

  while (cpi.Next()) {
    if (IsPercent(cpi.Current())) {
      if (!cpi.Next() || !IsHexCharacter(cpi.Current()) ||
          !cpi.Next() || !IsHexCharacter(cpi.Current())) {
        return str.raw();
      }
      num_escapes += 2;
    } 
  }

   ASSERT(len - num_escapes > 0);
   const String& dststr = String::Handle(
       OneByteString::New(len - num_escapes, Heap::kNew));
@@ -14563,7 +14598,7 @@ RawString* String::DecodeURI(const String& str) {
     while (cpi.Next()) {
       int32_t code_point = cpi.Current();
       if (IsPercent(code_point)) {
-        ASSERT(cpi.Next());
+        cpi.Next();
         int32_t ch1 = cpi.Current();
         cpi.Next();
         int32_t ch2 = cpi.Current();