[ubsan] Make Isolate inherit from Factory

Previously Isolate and Factory relied on the undefined behavior of
reinterpret_cast to switch between the two unrelated classes (which worked
because Factory had no data members).

With Isolate inheriting from Factory, it's now possible to switch between the
two classes using c-style casts. These are allowed under the C++ standard.

The inheritance is private which allows the continuing separation of the
Factory and Isolate namespaces.

This is a defensive clean-up, since ubsan does not yet detect the previous
undefined behavior.

Bug: v8:3770
Change-Id: I0ccf09f1d34f747550812ce698ab7e182812409e
Reviewed-on: https://chromium-review.googlesource.com/1010122
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52615}

src/heap/factory.h

@@ -81,7 +81,7 @@ enum FunctionMode {
 };
 
 // Interface for handle based allocation.
-class V8_EXPORT_PRIVATE Factory final {
+class V8_EXPORT_PRIVATE Factory {
  public:
   Handle<Oddball> NewOddball(Handle<Map> map, const char* to_string,
                              Handle<Object> to_number, const char* type_of,
@@ -894,7 +894,12 @@ class V8_EXPORT_PRIVATE Factory final {
   }
 
  private:
-  Isolate* isolate() { return reinterpret_cast<Isolate*>(this); }
+  Isolate* isolate() {
+    // Downcast to the privately inherited sub-class using c-style casts to
+    // avoid undefined behavior (as static_cast cannot cast across private
+    // bases).
+    return (Isolate*)this;  // NOLINT(readability/casting)
+  }
 
   HeapObject* AllocateRawWithImmortalMap(
       int size, PretenureFlag pretenure, Map* map,

src/isolate.h

@@ -23,6 +23,7 @@
 #include "src/futex-emulation.h"
 #include "src/globals.h"
 #include "src/handles.h"
+#include "src/heap/factory.h"
 #include "src/heap/heap.h"
 #include "src/messages.h"
 #include "src/objects/code.h"
@@ -72,7 +73,6 @@ class DescriptorLookupCache;
 class EmptyStatement;
 class EternalHandles;
 class ExternalCallbackScope;
-class Factory;
 class HandleScopeImplementer;
 class HeapObjectToIndexHashMap;
 class HeapProfiler;
@@ -454,8 +454,11 @@ typedef std::vector<HeapObject*> DebugObjectCache;
 #define THREAD_LOCAL_TOP_ADDRESS(type, name) \
   type* name##_address() { return &thread_local_top_.name##_; }
 
+// HiddenFactory exists so Isolate can privately inherit from it without making
+// Factory's members available to Isolate directly.
+class V8_EXPORT_PRIVATE HiddenFactory : private Factory {};
 
-class Isolate {
+class Isolate : private HiddenFactory {
   // These forward declarations are required to make the friend declarations in
   // PerIsolateThreadData work on some older versions of gcc.
   class ThreadDataTable;
@@ -983,7 +986,11 @@ class Isolate {
   }
 #endif
 
-  Factory* factory() { return reinterpret_cast<Factory*>(this); }
+  v8::internal::Factory* factory() {
+    // Upcast to the privately inherited base-class using c-style casts to avoid
+    // undefined behavior (as static_cast cannot cast across private bases).
+    return (v8::internal::Factory*)this;  // NOLINT(readability/casting)
+  }
 
   static const int kJSRegexpStaticOffsetsVectorSize = 128;