blink/bindings: Fix UB in StringResource.

v8::string::ExternalStringResourceBase -> StringResource{8,16} cast is undefined
behavior, as there is now ParkableStringResource{8,16} and
StringResource{8,16}.

Add a common ancestor to these two classes, and cast to it to avoid UB.

Bug: 909796
Change-Id: I94c65e03f283ca3c9fee344743e3f607686d2846
Reviewed-on: https://chromium-review.googlesource.com/c/1355046
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612250}

third_party/blink/renderer/platform/bindings/string_resource.cc

@@ -4,6 +4,8 @@
 
 #include "third_party/blink/renderer/platform/bindings/string_resource.h"
 
+#include <type_traits>
+
 #include "third_party/blink/renderer/platform/bindings/v8_binding.h"
 
 namespace blink {
@@ -92,11 +94,28 @@ StringType ToBlinkString(v8::Local<v8::String> v8_string,
     v8::String::ExternalStringResourceBase* resource =
         v8_string->GetExternalStringResourceBase(&encoding);
     if (LIKELY(!!resource)) {
+      // Inheritance:
+      // - V8 side: v8::String::ExternalStringResourceBase
+      //   -> v8::External{One,}ByteStringResource
+      // - Both: StringResource{8,16}Base inherits from the matching v8 class.
+      static_assert(std::is_base_of<v8::String::ExternalOneByteStringResource,
+                                    StringResource8Base>::value,
+                    "");
+      static_assert(std::is_base_of<v8::String::ExternalStringResource,
+                                    StringResource16Base>::value,
+                    "");
+      static_assert(
+          std::is_base_of<StringResourceBase, StringResource8Base>::value, "");
+      static_assert(
+          std::is_base_of<StringResourceBase, StringResource16Base>::value, "");
+      // Then StringResource{8,16}Base allows to go from one ancestry path to
+      // the other one. Even though it's empty, removing it causes UB, see
+      // crbug.com/909796.
       StringResourceBase* base;
       if (encoding == v8::String::ONE_BYTE_ENCODING)
-        base = static_cast<StringResource8*>(resource);
+        base = static_cast<StringResource8Base*>(resource);
       else
-        base = static_cast<StringResource16*>(resource);
+        base = static_cast<StringResource16Base*>(resource);
       return StringTraits<StringType>::FromStringResource(base);
     }
   }

third_party/blink/renderer/platform/bindings/string_resource.h

@@ -114,55 +114,53 @@ class StringResourceBase {
   DISALLOW_COPY_AND_ASSIGN(StringResourceBase);
 };
 
-class StringResource16 final : public StringResourceBase,
-                               public v8::String::ExternalStringResource {
+// Even though StringResource{8,16}Base are effectively empty in release mode,
+// they are needed as they serve as a common ancestor to Parkable and regular
+// strings.
+//
+// See the comment in |ToBlinkString()|'s implementation for the rationale.
+class StringResource16Base : public StringResourceBase,
+                             public v8::String::ExternalStringResource {
  public:
-  explicit StringResource16(const String& string) : StringResourceBase(string) {
+  explicit StringResource16Base(const String& string)
+      : StringResourceBase(string) {
     DCHECK(!string.Is8Bit());
   }
 
-  explicit StringResource16(const AtomicString& string)
+  explicit StringResource16Base(const AtomicString& string)
       : StringResourceBase(string) {
     DCHECK(!string.Is8Bit());
   }
 
-  size_t length() const override { return plain_string_.Impl()->length(); }
-  const uint16_t* data() const override {
-    return reinterpret_cast<const uint16_t*>(
-        plain_string_.Impl()->Characters16());
+  explicit StringResource16Base(const ParkableString& parkable_string)
+      : StringResourceBase(parkable_string) {
+    DCHECK(!parkable_string.Is8Bit());
   }
 
-  DISALLOW_COPY_AND_ASSIGN(StringResource16);
+  DISALLOW_COPY_AND_ASSIGN(StringResource16Base);
 };
 
-class StringResource8 final : public StringResourceBase,
-                              public v8::String::ExternalOneByteStringResource {
+class StringResource16 final : public StringResource16Base {
  public:
-  explicit StringResource8(const String& string) : StringResourceBase(string) {
-    DCHECK(string.Is8Bit());
-  }
+  explicit StringResource16(const String& string)
+      : StringResource16Base(string) {}
 
-  explicit StringResource8(const AtomicString& string)
-      : StringResourceBase(string) {
-    DCHECK(string.Is8Bit());
-  }
+  explicit StringResource16(const AtomicString& string)
+      : StringResource16Base(string) {}
 
   size_t length() const override { return plain_string_.Impl()->length(); }
-  const char* data() const override {
-    return reinterpret_cast<const char*>(plain_string_.Impl()->Characters8());
+  const uint16_t* data() const override {
+    return reinterpret_cast<const uint16_t*>(
+        plain_string_.Impl()->Characters16());
   }
 
-  DISALLOW_COPY_AND_ASSIGN(StringResource8);
+  DISALLOW_COPY_AND_ASSIGN(StringResource16);
 };
 
-class ParkableStringResource16 final
-    : public StringResourceBase,
-      public v8::String::ExternalStringResource {
+class ParkableStringResource16 final : public StringResource16Base {
  public:
   explicit ParkableStringResource16(const ParkableString& string)
-      : StringResourceBase(string) {
-    DCHECK(!parkable_string_.Is8Bit());
-  }
+      : StringResource16Base(string) {}
 
   bool IsCacheable() const override {
     return !parkable_string_.may_be_parked();
@@ -181,15 +179,48 @@ class ParkableStringResource16 final
   DISALLOW_COPY_AND_ASSIGN(ParkableStringResource16);
 };
 
-class ParkableStringResource8 final
-    : public StringResourceBase,
-      public v8::String::ExternalOneByteStringResource {
+class StringResource8Base : public StringResourceBase,
+                            public v8::String::ExternalOneByteStringResource {
  public:
-  explicit ParkableStringResource8(const ParkableString& string)
+  explicit StringResource8Base(const String& string)
       : StringResourceBase(string) {
-    DCHECK(parkable_string_.Is8Bit());
+    DCHECK(string.Is8Bit());
+  }
+
+  explicit StringResource8Base(const AtomicString& string)
+      : StringResourceBase(string) {
+    DCHECK(string.Is8Bit());
+  }
+
+  explicit StringResource8Base(const ParkableString& parkable_string)
+      : StringResourceBase(parkable_string) {
+    DCHECK(parkable_string.Is8Bit());
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(StringResource8Base);
+};
+
+class StringResource8 final : public StringResource8Base {
+ public:
+  explicit StringResource8(const String& string)
+      : StringResource8Base(string) {}
+
+  explicit StringResource8(const AtomicString& string)
+      : StringResource8Base(string) {}
+
+  size_t length() const override { return plain_string_.Impl()->length(); }
+  const char* data() const override {
+    return reinterpret_cast<const char*>(plain_string_.Impl()->Characters8());
   }
 
+  DISALLOW_COPY_AND_ASSIGN(StringResource8);
+};
+
+class ParkableStringResource8 final : public StringResource8Base {
+ public:
+  explicit ParkableStringResource8(const ParkableString& string)
+      : StringResource8Base(string) {}
+
   bool IsCacheable() const override {
     return !parkable_string_.may_be_parked();
   }