Fix release mode crash in String::DecodeURI.

runtime/vm/object.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/object.h"
 
 #include "include/dart_api.h"
 #include "platform/assert.h"
 #include "vm/assembler.h"
 #include "vm/cpu.h"
@@ skipping 14444 matching lines @@
   return TwoByteString::EscapeSpecialCharacters(
       String::Handle(TwoByteString::New(str, Heap::kNew)));
 }
 
 
 static bool IsPercent(int32_t c) {
   return 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;
   }
   if ((c >= 'a') && (c <= 'z')) {
     return true;
   }
   if ((c >= 'A') && (c <= 'Z')) {
     return true;
   }
@@ skipping 63 matching lines @@
   return dststr.raw();
 }
 
 
 RawString* String::DecodeURI(const String& str) {
   // URI encoding is only specified for one byte strings.
   ASSERT(str.IsOneByteString() || str.IsExternalOneByteString());
   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));
   {
     intptr_t index = 0;
     CodePointIterator cpi(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();
         int32_t merged = MergeHexCharacters(ch1, ch2);
         OneByteString::SetCharAt(dststr, index, merged);
       } else {
         OneByteString::SetCharAt(dststr, index, code_point);
       }
       index++;
     }
@@ skipping 2179 matching lines @@
   return "_MirrorReference";
 }
 
 
 void MirrorReference::PrintToJSONStream(JSONStream* stream, bool ref) const {
   Instance::PrintToJSONStream(stream, ref);
 }
 
 
 }  // namespace dart