[vm] Avoid UB in CompressedStackMap payloads

Having uint32_t field in UntaggedCompressedStackMaps::Payload
implies 4 byte alignment of the whole structure and the field
itself. We would prefer, however, to avoid this requirement because
we are packing these structures tight in AOT snapshots without any
padding in between.

This change rewrites the implementation to use `memcpy(...)` to remove
any alignment requirements. Note that this `memcpy(...)` will
be optimized down to a single memory move on all platforms we currently
support as they support necessary unaligned load/store instructions.

This also means that the original code worked just fine, except when
running under UBSAN.

Fixes dart-lang/sdk#47971

TEST=ubsan bots

Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-ubsan-linux-release-x64-try,vm-kernel-ubsan-linux-release-x64-try
Change-Id: I26a30123fcbc6d2c711f039f15f749913ce4d7bd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/226100
Reviewed-by: Alexander Aprelev <[email protected]>
Commit-Queue: Slava Egorov <[email protected]>

Author: mraleph

runtime/platform/globals.h

@@ -653,7 +653,7 @@ static inline void USE(T&&) {}
 // gcc enough that it can no longer see that you have cast one pointer
 // type to another thus avoiding the warning.
 template <class D, class S>
-inline D bit_cast(const S& source) {
+DART_FORCE_INLINE D bit_cast(const S& source) {
   static_assert(sizeof(D) == sizeof(S),
                 "Source and destination must have the same size");
 
@@ -669,7 +669,7 @@ inline D bit_cast(const S& source) {
 // obscure details in the C++ standard that make reinterpret_cast
 // virtually useless.
 template <class D, class S>
-inline D bit_copy(const S& source) {
+DART_FORCE_INLINE D bit_copy(const S& source) {
   D destination;
   // This use of memcpy is safe: source and destination cannot overlap.
   memcpy(&destination, reinterpret_cast<const void*>(&source),

runtime/vm/app_snapshot.cc

@@ -2820,7 +2820,7 @@ class CompressedStackMapsSerializationCluster : public SerializationCluster {
       s->AssignRef(map);
       AutoTraceObject(map);
       const intptr_t length = UntaggedCompressedStackMaps::SizeField::decode(
-          map->untag()->payload()->flags_and_size);
+          map->untag()->payload()->flags_and_size());
       s->WriteUnsigned(length);
       target_memory_size_ +=
           compiler::target::CompressedStackMaps::InstanceSize(length);
@@ -2832,9 +2832,9 @@ class CompressedStackMapsSerializationCluster : public SerializationCluster {
     for (intptr_t i = 0; i < count; i++) {
       CompressedStackMapsPtr map = objects_[i];
       AutoTraceObject(map);
-      s->WriteUnsigned(map->untag()->payload()->flags_and_size);
+      s->WriteUnsigned(map->untag()->payload()->flags_and_size());
       const intptr_t length = UntaggedCompressedStackMaps::SizeField::decode(
-          map->untag()->payload()->flags_and_size);
+          map->untag()->payload()->flags_and_size());
       uint8_t* cdata =
           reinterpret_cast<uint8_t*>(map->untag()->payload()->data());
       s->WriteBytes(cdata, length);
@@ -2874,7 +2874,7 @@ class CompressedStackMapsDeserializationCluster
           static_cast<CompressedStackMapsPtr>(d->Ref(id));
       Deserializer::InitializeHeader(map, kCompressedStackMapsCid,
                                      CompressedStackMaps::InstanceSize(length));
-      map->untag()->payload()->flags_and_size = flags_and_size;
+      map->untag()->payload()->set_flags_and_size(flags_and_size);
       uint8_t* cdata =
           reinterpret_cast<uint8_t*>(map->untag()->payload()->data());
       d->ReadBytes(cdata, length);
@@ -6978,7 +6978,7 @@ void Serializer::PrepareInstructions(
 
     // Now write collected stack maps after the binary search table.
     auto write_stack_map = [&](CompressedStackMapsPtr smap) {
-      const auto flags_and_size = smap->untag()->payload()->flags_and_size;
+      const auto flags_and_size = smap->untag()->payload()->flags_and_size();
       const auto payload_size =
           UntaggedCompressedStackMaps::SizeField::decode(flags_and_size);
       pc_mapping.WriteFixed<uint32_t>(flags_and_size);

runtime/vm/compiler/runtime_offsets_list.h

@@ -350,7 +350,7 @@
   SIZEOF(CodeSourceMap, HeaderSize, UntaggedCodeSourceMap)                     \
   SIZEOF(CompressedStackMaps, ObjectHeaderSize, UntaggedCompressedStackMaps)   \
   SIZEOF(CompressedStackMaps, PayloadHeaderSize,                               \
-         UntaggedCompressedStackMaps::Payload)                                 \
+         UntaggedCompressedStackMaps::Payload::FlagsAndSizeHeader)             \
   SIZEOF(Context, header_size, UntaggedContext)                                \
   SIZEOF(Double, InstanceSize, UntaggedDouble)                                 \
   SIZEOF(DynamicLibrary, InstanceSize, UntaggedDynamicLibrary)                 \

runtime/vm/image_snapshot.cc

@@ -529,7 +529,7 @@ void ImageWriter::WriteROData(NonStreamingWriteStream* stream, bool vm) {
       const CompressedStackMaps& map = CompressedStackMaps::Cast(obj);
       const intptr_t payload_size = map.payload_size();
       stream->WriteFixed<uint32_t>(
-          map.ptr()->untag()->payload()->flags_and_size);
+          map.ptr()->untag()->payload()->flags_and_size());
       stream->WriteBytes(map.ptr()->untag()->payload()->data(), payload_size);
     } else if (obj.IsCodeSourceMap()) {
       const CodeSourceMap& map = CodeSourceMap::Cast(obj);

runtime/vm/object.cc

@@ -1035,8 +1035,7 @@ void Object::Init(IsolateGroup* isolate_group) {
     CompressedStackMaps::initializeHandle(
         empty_compressed_stackmaps_,
         static_cast<CompressedStackMapsPtr>(address + kHeapObjectTag));
-    empty_compressed_stackmaps_->StoreNonPointer(
-        &empty_compressed_stackmaps_->untag()->payload()->flags_and_size, 0);
+    empty_compressed_stackmaps_->untag()->payload()->set_flags_and_size(0);
     empty_compressed_stackmaps_->SetCanonical();
   }
 
@@ -15074,12 +15073,10 @@ CompressedStackMapsPtr CompressedStackMaps::New(const void* payload,
         Heap::kOld, CompressedStackMaps::ContainsCompressedPointers());
     NoSafepointScope no_safepoint;
     result ^= raw;
-    result.StoreNonPointer(
-        &result.untag()->payload()->flags_and_size,
+    result.untag()->payload()->set_flags_and_size(
         UntaggedCompressedStackMaps::GlobalTableBit::encode(is_global_table) |
-            UntaggedCompressedStackMaps::UsesTableBit::encode(
-                uses_global_table) |
-            UntaggedCompressedStackMaps::SizeField::encode(size));
+        UntaggedCompressedStackMaps::UsesTableBit::encode(uses_global_table) |
+        UntaggedCompressedStackMaps::SizeField::encode(size));
     auto cursor =
         result.UnsafeMutableNonPointer(result.untag()->payload()->data());
     memcpy(cursor, payload, size);  // NOLINT

runtime/vm/object.h

@@ -5883,16 +5883,16 @@ class CompressedStackMaps : public Object {
   uintptr_t payload_size() const { return PayloadSizeOf(ptr()); }
   static uintptr_t PayloadSizeOf(const CompressedStackMapsPtr raw) {
     return UntaggedCompressedStackMaps::SizeField::decode(
-        raw->untag()->payload()->flags_and_size);
+        raw->untag()->payload()->flags_and_size());
   }
 
   const uint8_t* data() const { return ptr()->untag()->payload()->data(); }
 
   // Methods to allow use with PointerKeyValueTrait to create sets of CSMs.
   bool Equals(const CompressedStackMaps& other) const {
     // All of the table flags and payload size must match.
-    if (untag()->payload()->flags_and_size !=
-        other.untag()->payload()->flags_and_size) {
+    if (untag()->payload()->flags_and_size() !=
+        other.untag()->payload()->flags_and_size()) {
       return false;
     }
     NoSafepointScope no_safepoint;
@@ -5902,7 +5902,7 @@ class CompressedStackMaps : public Object {
 
   static intptr_t HeaderSize() {
     return sizeof(UntaggedCompressedStackMaps) +
-           sizeof(UntaggedCompressedStackMaps::Payload);
+           sizeof(UntaggedCompressedStackMaps::Payload::FlagsAndSizeHeader);
   }
   static intptr_t UnroundedSize(CompressedStackMapsPtr maps) {
     return UnroundedSize(CompressedStackMaps::PayloadSizeOf(maps));
@@ -5920,13 +5920,13 @@ class CompressedStackMaps : public Object {
   bool UsesGlobalTable() const { return UsesGlobalTable(ptr()); }
   static bool UsesGlobalTable(const CompressedStackMapsPtr raw) {
     return UntaggedCompressedStackMaps::UsesTableBit::decode(
-        raw->untag()->payload()->flags_and_size);
+        raw->untag()->payload()->flags_and_size());
   }
 
   bool IsGlobalTable() const { return IsGlobalTable(ptr()); }
   static bool IsGlobalTable(const CompressedStackMapsPtr raw) {
     return UntaggedCompressedStackMaps::GlobalTableBit::decode(
-        raw->untag()->payload()->flags_and_size);
+        raw->untag()->payload()->flags_and_size());
   }
 
   static CompressedStackMapsPtr NewInlined(const void* payload, intptr_t size) {
@@ -5976,18 +5976,18 @@ class CompressedStackMaps : public Object {
 
     uintptr_t payload_size() const {
       return UntaggedCompressedStackMaps::SizeField::decode(
-          payload()->flags_and_size);
+          payload()->flags_and_size());
     }
     const uint8_t* data() const { return payload()->data(); }
 
     bool UsesGlobalTable() const {
       return UntaggedCompressedStackMaps::UsesTableBit::decode(
-          payload()->flags_and_size);
+          payload()->flags_and_size());
     }
 
     bool IsGlobalTable() const {
       return UntaggedCompressedStackMaps::GlobalTableBit::decode(
-          payload()->flags_and_size);
+          payload()->flags_and_size());
     }
 
    private:

runtime/vm/raw_object.h

@@ -1987,11 +1987,26 @@ class UntaggedCompressedStackMaps : public UntaggedObject {
   VISIT_NOTHING();
 
  public:
-  struct Payload {
+  // Note: AOT snapshots pack these structures without any padding in between
+  // so payload structure should not have any alignment requirements.
+  // alignas(1) is here to trigger a compiler error if we violate this.
+  struct alignas(1) Payload {
+    using FlagsAndSizeHeader = uint32_t;
+
     // The most significant bits are the length of the encoded payload, in
     // bytes (excluding the header itself). The low bits determine the
     // expected payload contents, as described below.
-    uint32_t flags_and_size;
+    DART_FORCE_INLINE FlagsAndSizeHeader flags_and_size() const {
+      // Note: |this| does not necessarily satisfy alignment requirements
+      // of uint32_t so we should use bit_cast.
+      return bit_copy<FlagsAndSizeHeader, Payload>(*this);
+    }
+
+    DART_FORCE_INLINE void set_flags_and_size(FlagsAndSizeHeader value) {
+      // Note: |this| does not necessarily satisfy alignment requirements
+      // of uint32_t hence the byte copy below.
+      memcpy(reinterpret_cast<void*>(this), &value, sizeof(value));  // NOLINT
+    }
 
     // Variable length data follows here. The contents of the payload depend on
     // the type of CompressedStackMaps (CSM) being represented. There are three
@@ -2044,28 +2059,36 @@ class UntaggedCompressedStackMaps : public UntaggedObject {
     // done where the payload is decoded up to the entry whose PC offset
     // is greater or equal to the given PC.
 
-    uint8_t* data() { OPEN_ARRAY_START(uint8_t, uint8_t); }
-    const uint8_t* data() const { OPEN_ARRAY_START(uint8_t, uint8_t); }
+    uint8_t* data() {
+      return reinterpret_cast<uint8_t*>(this) + sizeof(FlagsAndSizeHeader);
+    }
+
+    const uint8_t* data() const {
+      return reinterpret_cast<const uint8_t*>(this) +
+             sizeof(FlagsAndSizeHeader);
+    }
   };
-  static_assert(sizeof(Payload) == sizeof(uint32_t));
 
  private:
   // We are using OPEN_ARRAY_START rather than embedding Payload directly into
   // the UntaggedCompressedStackMaps as a field because that would introduce a
   // padding at the end of UntaggedCompressedStackMaps - so we would not be
   // able to use sizeof(UntaggedCompressedStackMaps) as the size of the header
   // anyway.
-  Payload* payload() { OPEN_ARRAY_START(Payload, uint32_t); }
-  const Payload* payload() const { OPEN_ARRAY_START(Payload, uint32_t); }
-
-  class GlobalTableBit : public BitField<uint32_t, bool, 0, 1> {};
-  class UsesTableBit
-      : public BitField<uint32_t, bool, GlobalTableBit::kNextBit, 1> {};
+  Payload* payload() { OPEN_ARRAY_START(Payload, uint8_t); }
+  const Payload* payload() const { OPEN_ARRAY_START(Payload, uint8_t); }
+
+  class GlobalTableBit
+      : public BitField<Payload::FlagsAndSizeHeader, bool, 0, 1> {};
+  class UsesTableBit : public BitField<Payload::FlagsAndSizeHeader,
+                                       bool,
+                                       GlobalTableBit::kNextBit,
+                                       1> {};
   class SizeField
-      : public BitField<uint32_t,
-                        uint32_t,
+      : public BitField<Payload::FlagsAndSizeHeader,
+                        Payload::FlagsAndSizeHeader,
                         UsesTableBit::kNextBit,
-                        sizeof(Payload::flags_and_size) * kBitsPerByte -
+                        sizeof(Payload::FlagsAndSizeHeader) * kBitsPerByte -
                             UsesTableBit::kNextBit> {};
 
   friend class Object;