Actually fix the bug, which was a doozy: OrphanBuilder::tag was sometimes…

Actually fix the bug, which was a doozy:  OrphanBuilder::tag was sometimes initialized using WirePointer::setKindAndTarget(), but since the tag didn't live inside the target segment, this used illegal pointer arithmetic.  The target is never read from an orphan tag anyway, so I thought it would be no big deal.  But it turns out Clang actually optimizes under the assumption that pointer arithmetic returns a whole value.  As a result, on 32-bit system where 64-bit values are only 32-bit aligned, the tag and target might not have been a whole number of words apart, and the extra bit actually found its way into the 'kind' bits, causing e.g. a struct pointer to become an invalid far pointer.  Crash.  The fix required refactoring to ensure that setKindAndOffset() is never used for orphan tags.

c++/src/capnp/encoding-test.c++

@@ -1747,6 +1747,51 @@ TEST(Encoding, GlobalConstants) {
   }
 }
 
+TEST(Encoding, HasEmptyStruct) {
+  MallocMessageBuilder message;
+  auto root = message.initRoot<test::TestObject>();
+
+  EXPECT_EQ(1, root.totalSizeInWords());
+
+  EXPECT_FALSE(root.asReader().hasObjectField());
+  EXPECT_FALSE(root.hasObjectField());
+  root.initObjectField<test::TestEmptyStruct>();
+  EXPECT_TRUE(root.asReader().hasObjectField());
+  EXPECT_TRUE(root.hasObjectField());
+
+  EXPECT_EQ(1, root.totalSizeInWords());
+}
+
+TEST(Encoding, HasEmptyList) {
+  MallocMessageBuilder message;
+  auto root = message.initRoot<test::TestObject>();
+
+  EXPECT_EQ(1, root.totalSizeInWords());
+
+  EXPECT_FALSE(root.asReader().hasObjectField());
+  EXPECT_FALSE(root.hasObjectField());
+  root.initObjectField<List<int32_t>>(0);
+  EXPECT_TRUE(root.asReader().hasObjectField());
+  EXPECT_TRUE(root.hasObjectField());
+
+  EXPECT_EQ(1, root.totalSizeInWords());
+}
+
+TEST(Encoding, HasEmptyStructList) {
+  MallocMessageBuilder message;
+  auto root = message.initRoot<test::TestObject>();
+
+  EXPECT_EQ(1, root.totalSizeInWords());
+
+  EXPECT_FALSE(root.asReader().hasObjectField());
+  EXPECT_FALSE(root.hasObjectField());
+  root.initObjectField<List<TestAllTypes>>(0);
+  EXPECT_TRUE(root.asReader().hasObjectField());
+  EXPECT_TRUE(root.hasObjectField());
+
+  EXPECT_EQ(2, root.totalSizeInWords());
+}
+
 }  // namespace
 }  // namespace _ (private)
 }  // namespace capnp

c++/src/capnp/layout.h

@@ -279,6 +279,10 @@ public:
   static StructBuilder getRoot(SegmentBuilder* segment, word* location, StructSize size);
   static void adoptRoot(SegmentBuilder* segment, word* location, OrphanBuilder orphan);
 
+  inline word* getLocation() { return reinterpret_cast<word*>(data); }
+  // Get the object's location.  Only valid for independently-allocated objects (i.e. not list
+  // elements).
+
   inline BitCount getDataSectionSize() const { return dataSize; }
   inline WirePointerCount getPointerSectionSize() const { return pointerCount; }
   inline Data::Builder getDataSectionAsBlob();
@@ -533,6 +537,17 @@ public:
       : segment(nullptr), ptr(nullptr), elementCount(0 * ELEMENTS),
         step(0 * BITS / ELEMENTS) {}
 
+  inline word* getLocation() {
+    // Get the object's location.  Only valid for independently-allocated objects (i.e. not list
+    // elements).
+
+    if (step * ELEMENTS <= BITS_PER_WORD * WORDS) {
+      return reinterpret_cast<word*>(ptr);
+    } else {
+      return reinterpret_cast<word*>(ptr) - POINTER_SIZE_IN_WORDS;
+    }
+  }
+
   inline ElementCount size() const;
   // The number of elements in the list.
 
@@ -722,6 +737,15 @@ struct ObjectBuilder {
   ObjectBuilder(ListBuilder listBuilder)
       : kind(ObjectKind::LIST), listBuilder(listBuilder) {}
 
+  inline word* getLocation() {
+    switch (kind) {
+      case ObjectKind::NULL_POINTER: return nullptr;
+      case ObjectKind::STRUCT: return structBuilder.getLocation();
+      case ObjectKind::LIST: return listBuilder.getLocation();
+    }
+    return nullptr;
+  }
+
   ObjectReader asReader() const;
 
   inline ObjectBuilder(ObjectBuilder& other) { memcpy(this, &other, sizeof(*this)); }
@@ -773,8 +797,8 @@ public:
   OrphanBuilder& operator=(const OrphanBuilder& other) = delete;
   inline OrphanBuilder& operator=(OrphanBuilder&& other);
 
-  inline bool operator==(decltype(nullptr)) const { return segment == nullptr; }
-  inline bool operator!=(decltype(nullptr)) const { return segment != nullptr; }
+  inline bool operator==(decltype(nullptr)) const { return location == nullptr; }
+  inline bool operator!=(decltype(nullptr)) const { return location != nullptr; }
 
   StructBuilder asStruct(StructSize size);
   // Interpret as a struct, or throw an exception if not a struct.
@@ -816,8 +840,7 @@ private:
   // FAR pointer.
 
   word* location;
-  // Pointer to the object.  Invalid if the tag is a FAR pointer (in which case you need to follow
-  // the FAR pointer instead).
+  // Pointer to the object, or nullptr if the pointer is null.
 
   inline OrphanBuilder(const void* tagPtr, SegmentBuilder* segment, word* location)
       : segment(segment), location(location) {

c++/src/capnp/orphan-test.c++

@@ -781,12 +781,103 @@ TEST(Orphans, FarPointer) {
   EXPECT_TRUE(orphan != nullptr);
   EXPECT_FALSE(orphan == nullptr);
 
-  KJ_DBG(orphan != nullptr, orphan == nullptr);
-
   checkTestMessage(orphan.getReader());
   checkTestMessage(orphan.get());
 }
 
+TEST(Orphans, UpgradeStruct) {
+  MallocMessageBuilder builder;
+  auto root = builder.initRoot<test::TestObject>();
+
+  auto old = root.initObjectField<test::TestOldVersion>();
+  old.setOld1(1234);
+  old.setOld2("foo");
+
+  auto orphan = root.disownObjectField<test::TestNewVersion>();
+
+  // Relocation has not occurred yet.
+  old.setOld1(12345);
+  EXPECT_EQ(12345, orphan.getReader().getOld1());
+  EXPECT_EQ("foo", old.getOld2());
+
+  // This will relocate the struct.
+  auto newVersion = orphan.get();
+
+  EXPECT_EQ(0, old.getOld1());
+  EXPECT_EQ("", old.getOld2());
+
+  EXPECT_EQ(12345, newVersion.getOld1());
+  EXPECT_EQ("foo", newVersion.getOld2());
+}
+
+TEST(Orphans, UpgradeStructList) {
+  MallocMessageBuilder builder;
+  auto root = builder.initRoot<test::TestObject>();
+
+  auto old = root.initObjectField<List<test::TestOldVersion>>(2);
+  old[0].setOld1(1234);
+  old[0].setOld2("foo");
+  old[1].setOld1(4321);
+  old[1].setOld2("bar");
+
+  auto orphan = root.disownObjectField<List<test::TestNewVersion>>();
+
+  // Relocation has not occurred yet.
+  old[0].setOld1(12345);
+  EXPECT_EQ(12345, orphan.getReader()[0].getOld1());
+  EXPECT_EQ("foo", old[0].getOld2());
+
+  // This will relocate the struct.
+  auto newVersion = orphan.get();
+
+  EXPECT_EQ(0, old[0].getOld1());
+  EXPECT_EQ("", old[0].getOld2());
+
+  EXPECT_EQ(12345, newVersion[0].getOld1());
+  EXPECT_EQ("foo", newVersion[0].getOld2());
+  EXPECT_EQ(4321, newVersion[1].getOld1());
+  EXPECT_EQ("bar", newVersion[1].getOld2());
+}
+
+TEST(Orphans, DisownNull) {
+  MallocMessageBuilder builder;
+  auto root = builder.initRoot<TestAllTypes>();
+
+  {
+    Orphan<TestAllTypes> orphan = root.disownStructField();
+    EXPECT_TRUE(orphan == nullptr);
+
+    checkTestMessageAllZero(orphan.getReader());
+    EXPECT_TRUE(orphan == nullptr);
+
+    // get()ing the orphan allocates an object, for security reasons.
+    checkTestMessageAllZero(orphan.get());
+    EXPECT_FALSE(orphan == nullptr);
+  }
+
+  {
+    Orphan<List<int32_t>> orphan = root.disownInt32List();
+    EXPECT_TRUE(orphan == nullptr);
+
+    EXPECT_EQ(0, orphan.getReader().size());
+    EXPECT_TRUE(orphan == nullptr);
+
+    EXPECT_EQ(0, orphan.get().size());
+    EXPECT_TRUE(orphan == nullptr);
+  }
+
+  {
+    Orphan<List<TestAllTypes>> orphan = root.disownStructList();
+    EXPECT_TRUE(orphan == nullptr);
+
+    EXPECT_EQ(0, orphan.getReader().size());
+    EXPECT_TRUE(orphan == nullptr);
+
+    EXPECT_EQ(0, orphan.get().size());
+    EXPECT_TRUE(orphan == nullptr);
+  }
+}
+
 }  // namespace
 }  // namespace _ (private)
 }  // namespace capnp

c++/src/capnp/orphan.h

@@ -53,6 +53,12 @@ public:
   Orphan& operator=(Orphan&&) = default;
 
   inline typename T::Builder get();
+  // Get the underlying builder.  If the orphan is null, this will allocate and return a default
+  // object rather than crash.  This is done for security -- otherwise, you might enable a DoS
+  // attack any time you disown a field and fail to check if it is null.  In the case of structs,
+  // this means that the orphan is no longer null after get() returns.  In the case of lists,
+  // no actual object is allocated since a simple empty ListBuilder can be returned.
+
   inline typename T::Reader getReader() const;
 
   inline bool operator==(decltype(nullptr)) const { return builder == nullptr; }

c++/src/capnp/test.capnp

@@ -162,6 +162,9 @@ struct TestDefaults {
 
 struct TestObject {
   objectField @0 :Object;
+
+  # Do not add any other fields here!  Some tests rely on objectField being the last pointer
+  # in the struct.
 }
 
 struct TestOutOfOrder {
@@ -478,6 +481,8 @@ struct TestStructUnion {
   }
 }
 
+struct TestEmptyStruct {}
+
 struct TestConstants {
   const voidConst      :Void    = void;
   const boolConst      :Bool    = true;