Make FieldType::None() non-nullptr value to avoid undefined behaviour

addaleax · Commit bot · commit 8ed65b97a330 · 2016-08-30T17:56:05.000Z
When FieldType::None() returns a cast Smi::FromInt(0), which translates as nullptr, the FieldType::IsNone() check becomes equivalent to `this == nullptr` which is not allowed by the standard and therefore optimized away as a false constant by GCC 6. This has lead to crashes when invoking methods on FieldType::None(). Using a different Smi constant for FieldType::None() makes the compiler always include a comparison against that value. The choice of these constants has no effect as they are effectively arbitrary. BUG=nodejs/node#8310 Review-Url: https://codereview.chromium.org/2292953002 Cr-Commit-Position: refs/heads/master@{#39023}
diff --git a/src/field-type.cc b/src/field-type.cc
@@ -13,7 +13,9 @@ namespace internal {
 
 // static
 FieldType* FieldType::None() {
-  return reinterpret_cast<FieldType*>(Smi::FromInt(0));
+  // Do not Smi::FromInt(0) here or for Any(), as that may translate
+  // as `nullptr` which is not a valid value for `this`.
+  return reinterpret_cast<FieldType*>(Smi::FromInt(2));
 }
 
 // static
diff --git a/test/cctest/test-field-type-tracking.cc b/test/cctest/test-field-type-tracking.cc
@@ -17,6 +17,7 @@
 #include "src/global-handles.h"
 #include "src/ic/stub-cache.h"
 #include "src/macro-assembler.h"
+#include "src/types.h"
 
 using namespace v8::internal;
 
@@ -2410,6 +2411,16 @@ TEST(TransitionAccessorConstantToSameAccessorConstant) {
   TestTransitionTo(transition_op, transition_op, checker);
 }
 
+TEST(FieldTypeConvertSimple) {
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+  Isolate* isolate = CcTest::i_isolate();
+
+  Zone zone(isolate->allocator());
+
+  CHECK_EQ(FieldType::Any()->Convert(&zone), Type::NonInternal());
+  CHECK_EQ(FieldType::None()->Convert(&zone), Type::None());
+}
 
 // TODO(ishell): add this test once IS_ACCESSOR_FIELD_SUPPORTED is supported.
 // TEST(TransitionAccessorConstantToAnotherAccessorConstant)

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,9 @@ namespace internal {`
`13`	`13`
`14`	`14`	`// static`
`15`	`15`	`FieldType* FieldType::None() {`
`16`		`- return reinterpret_cast<FieldType*>(Smi::FromInt(0));`
	`16`	`+ // Do not Smi::FromInt(0) here or for Any(), as that may translate`
	`17`	+ // as `nullptr` which is not a valid value for `this`.
	`18`	`+ return reinterpret_cast<FieldType*>(Smi::FromInt(2));`
`17`	`19`	`}`
`18`	`20`
`19`	`21`	`// static`