From 5c409566e9c230fdd39d7ce1cb78646f23886728 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 23 Jun 2024 20:20:32 +0200 Subject: [PATCH 1/3] Revert "src: make sure that memcpy-ed structs in snapshot have no padding" This reverts commit 4e58cde589dfd980c8976b158853a331142e1e4b. --- src/aliased_buffer-inl.h | 2 +- src/aliased_buffer.h | 2 +- src/base_object_types.h | 7 ++----- src/encoding_binding.h | 6 ------ src/node_file.h | 6 ------ src/node_process.h | 6 ------ src/node_snapshotable.cc | 12 ++++++------ src/node_snapshotable.h | 37 ++++--------------------------------- src/node_v8.h | 7 ------- 9 files changed, 14 insertions(+), 71 deletions(-) diff --git a/src/aliased_buffer-inl.h b/src/aliased_buffer-inl.h index 40e08dfe149667..f04fb7c2667016 100644 --- a/src/aliased_buffer-inl.h +++ b/src/aliased_buffer-inl.h @@ -8,7 +8,7 @@ namespace node { -typedef uint64_t AliasedBufferIndex; +typedef size_t AliasedBufferIndex; template AliasedBufferBase::AliasedBufferBase( diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h index c2ddd269fd179c..b847641f8faa15 100644 --- a/src/aliased_buffer.h +++ b/src/aliased_buffer.h @@ -9,7 +9,7 @@ namespace node { -typedef uint64_t AliasedBufferIndex; +typedef size_t AliasedBufferIndex; /** * Do not use this class directly when creating instances of it - use the diff --git a/src/base_object_types.h b/src/base_object_types.h index 391a88973171ab..9cfe6a77f71708 100644 --- a/src/base_object_types.h +++ b/src/base_object_types.h @@ -42,13 +42,10 @@ namespace node { SERIALIZABLE_NON_BINDING_TYPES(V) #define V(TypeId, NativeType) k_##TypeId, -// To avoid padding, the enums are uint64_t. -enum class BindingDataType : uint64_t { - BINDING_TYPES(V) kBindingDataTypeCount -}; +enum class BindingDataType : uint8_t { BINDING_TYPES(V) kBindingDataTypeCount }; // Make sure that we put the bindings first so that we can also use the enums // for the bindings as index to the binding data store. -enum class EmbedderObjectType : uint64_t { +enum class EmbedderObjectType : uint8_t { BINDING_TYPES(V) SERIALIZABLE_NON_BINDING_TYPES(V) // We do not need to know about all the unserializable non-binding types for // now so we do not list them. diff --git a/src/encoding_binding.h b/src/encoding_binding.h index 7b18db9c6a1108..2690cb74f8a05b 100644 --- a/src/encoding_binding.h +++ b/src/encoding_binding.h @@ -18,12 +18,6 @@ class BindingData : public SnapshotableObject { AliasedBufferIndex encode_into_results_buffer; }; - // Make sure that there's no padding in the struct since we will memcpy - // them into the snapshot blob and they need to be reproducible. - static_assert(sizeof(InternalFieldInfo) == - sizeof(InternalFieldInfoBase) + sizeof(AliasedBufferIndex), - "InternalFieldInfo should have no padding"); - BindingData(Realm* realm, v8::Local obj, InternalFieldInfo* info = nullptr); diff --git a/src/node_file.h b/src/node_file.h index a0144314b280c1..bdad1ae25f4892 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -64,12 +64,6 @@ class BindingData : public SnapshotableObject { AliasedBufferIndex statfs_field_bigint_array; }; - // Make sure that there's no padding in the struct since we will memcpy - // them into the snapshot blob and they need to be reproducible. - static_assert(sizeof(InternalFieldInfo) == sizeof(InternalFieldInfoBase) + - sizeof(AliasedBufferIndex) * 4, - "InternalFieldInfo should have no padding"); - enum class FilePathIsFileReturnType { kIsFile = 0, kIsNotFile, diff --git a/src/node_process.h b/src/node_process.h index 607826398beeae..142d0e63e18c46 100644 --- a/src/node_process.h +++ b/src/node_process.h @@ -54,12 +54,6 @@ class BindingData : public SnapshotableObject { AliasedBufferIndex hrtime_buffer; }; - // Make sure that there's no padding in the struct since we will memcpy - // them into the snapshot blob and they need to be reproducible. - static_assert(sizeof(InternalFieldInfo) == - sizeof(InternalFieldInfoBase) + sizeof(AliasedBufferIndex), - "InternalFieldInfo should have no padding"); - static void AddMethods(v8::Isolate* isolate, v8::Local target); static void RegisterExternalReferences(ExternalReferenceRegistry* registry); diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index eaeafd8e16a91c..d2ccbe8429048a 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -282,9 +282,9 @@ size_t SnapshotSerializer::Write(const PropInfo& data) { } // Layout of AsyncHooks::SerializeInfo -// [ 8 bytes ] snapshot index of async_ids_stack -// [ 8 bytes ] snapshot index of fields -// [ 8 bytes ] snapshot index of async_id_fields +// [ 4/8 bytes ] snapshot index of async_ids_stack +// [ 4/8 bytes ] snapshot index of fields +// [ 4/8 bytes ] snapshot index of async_id_fields // [ 4/8 bytes ] snapshot index of js_execution_async_resources // [ 4/8 bytes ] length of native_execution_async_resources // [ ... ] snapshot indices of each element in @@ -387,9 +387,9 @@ size_t SnapshotSerializer::Write(const ImmediateInfo::SerializeInfo& data) { } // Layout of PerformanceState::SerializeInfo -// [ 8 bytes ] snapshot index of root -// [ 8 bytes ] snapshot index of milestones -// [ 8 bytes ] snapshot index of observers +// [ 4/8 bytes ] snapshot index of root +// [ 4/8 bytes ] snapshot index of milestones +// [ 4/8 bytes ] snapshot index of observers template <> performance::PerformanceState::SerializeInfo SnapshotDeserializer::Read() { Debug("Read()\n"); diff --git a/src/node_snapshotable.h b/src/node_snapshotable.h index 51a9499da5f2d7..600e56c481be90 100644 --- a/src/node_snapshotable.h +++ b/src/node_snapshotable.h @@ -4,8 +4,6 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include // For static_assert -#include // For offsetof #include "aliased_buffer.h" #include "base_object.h" #include "util.h" @@ -35,13 +33,13 @@ bool WithoutCodeCache(const SnapshotConfig& config); // and pass it into the V8 callback as the payload of StartupData. // The memory chunk looks like this: // -// [ type ] - EmbedderObjectType (a uint64_t) -// [ length ] - a uint64_t +// [ type ] - EmbedderObjectType (a uint8_t) +// [ length ] - a size_t // [ ... ] - custom bytes of size |length - header size| struct InternalFieldInfoBase { public: EmbedderObjectType type; - uint64_t length; + size_t length; template static T* New(EmbedderObjectType type) { @@ -73,35 +71,14 @@ struct InternalFieldInfoBase { InternalFieldInfoBase() = default; }; -// Make sure that there's no padding in the struct since we will memcpy -// them into the snapshot blob and they need to be reproducible. -static_assert(offsetof(InternalFieldInfoBase, type) == 0, - "InternalFieldInfoBase::type should start from offset 0"); -static_assert(offsetof(InternalFieldInfoBase, length) == - sizeof(EmbedderObjectType), - "InternalFieldInfoBase::type should have no padding"); - struct EmbedderTypeInfo { - // To avoid padding, the enum is uint64_t. - enum class MemoryMode : uint64_t { kBaseObject = 0, kCppGC }; + enum class MemoryMode : uint8_t { kBaseObject, kCppGC }; EmbedderTypeInfo(EmbedderObjectType t, MemoryMode m) : type(t), mode(m) {} EmbedderTypeInfo() = default; - EmbedderObjectType type; MemoryMode mode; }; -// Make sure that there's no padding in the struct since we will memcpy -// them into the snapshot blob and they need to be reproducible. -static_assert(offsetof(EmbedderTypeInfo, type) == 0, - "EmbedderTypeInfo::type should start from offset 0"); -static_assert(offsetof(EmbedderTypeInfo, mode) == sizeof(EmbedderObjectType), - "EmbedderTypeInfo::type should have no padding"); -static_assert(sizeof(EmbedderTypeInfo) == - sizeof(EmbedderObjectType) + - sizeof(EmbedderTypeInfo::MemoryMode), - "EmbedderTypeInfo::mode should have no padding"); - // An interface for snapshotable native objects to inherit from. // Use the SERIALIZABLE_OBJECT_METHODS() macro in the class to define // the following methods to implement: @@ -173,12 +150,6 @@ class BindingData : public SnapshotableObject { AliasedBufferIndex is_building_snapshot_buffer; }; - // Make sure that there's no padding in the struct since we will memcpy - // them into the snapshot blob and they need to be reproducible. - static_assert(sizeof(InternalFieldInfo) == - sizeof(InternalFieldInfoBase) + sizeof(AliasedBufferIndex), - "InternalFieldInfo should have no padding"); - BindingData(Realm* realm, v8::Local obj, InternalFieldInfo* info = nullptr); diff --git a/src/node_v8.h b/src/node_v8.h index 844addd6a93b1b..d3797432a24db7 100644 --- a/src/node_v8.h +++ b/src/node_v8.h @@ -23,13 +23,6 @@ class BindingData : public SnapshotableObject { AliasedBufferIndex heap_space_statistics_buffer; AliasedBufferIndex heap_code_statistics_buffer; }; - - // Make sure that there's no padding in the struct since we will memcpy - // them into the snapshot blob and they need to be reproducible. - static_assert(sizeof(InternalFieldInfo) == sizeof(InternalFieldInfoBase) + - sizeof(AliasedBufferIndex) * 3, - "InternalFieldInfo should have no padding"); - BindingData(Realm* realm, v8::Local obj, InternalFieldInfo* info = nullptr); From af91d7d98fa80433d877c54e45158ae8819291a8 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 23 Jun 2024 21:36:10 +0200 Subject: [PATCH 2/3] src: zero-initialize data that are copied into the snapshot To prevent padding from making the snapshot unreproducible, zero-initialize the data that are copied into the snapshot so that the padding copied are all zeros. This is better than enlarging the enums to align the fields since it doesn't make the snapshot bigger than necessary, and it removes the need of using static assertions to ensure alignment. --- src/node_snapshotable.cc | 1 + src/node_snapshotable.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index d2ccbe8429048a..9d5fdda61eb6d1 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1416,6 +1416,7 @@ StartupData SerializeNodeContextInternalFields(Local holder, if (index == BaseObject::kEmbedderType) { int size = sizeof(EmbedderTypeInfo); char* data = new char[size]; + memset(data, 0, size); // Make the padding reproducible. // We need to use placement new because V8 calls delete[] on the returned // data. // TODO(joyeecheung): support cppgc objects. diff --git a/src/node_snapshotable.h b/src/node_snapshotable.h index 600e56c481be90..31be74bcfd568a 100644 --- a/src/node_snapshotable.h +++ b/src/node_snapshotable.h @@ -47,6 +47,7 @@ struct InternalFieldInfoBase { std::is_same_v, "Can only accept InternalFieldInfoBase subclasses"); void* buf = ::operator new[](sizeof(T)); + memset(buf, 0, sizeof(T)); // Make the padding reproducible. T* result = new (buf) T; result->type = type; result->length = sizeof(T); From e99f06c925e7adfb770576c0306b1b6280fb3332 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 5 Jul 2024 21:57:53 +0200 Subject: [PATCH 3/3] fixup! src: zero-initialize data that are copied into the snapshot --- src/node_snapshotable.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 9d5fdda61eb6d1..426066fb9d63d4 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1415,10 +1415,11 @@ StartupData SerializeNodeContextInternalFields(Local holder, // To serialize the type field, save data in a EmbedderTypeInfo. if (index == BaseObject::kEmbedderType) { int size = sizeof(EmbedderTypeInfo); - char* data = new char[size]; - memset(data, 0, size); // Make the padding reproducible. // We need to use placement new because V8 calls delete[] on the returned // data. + // The () syntax at the end would zero-initialize the block and make + // the padding reproducible. + char* data = new char[size](); // TODO(joyeecheung): support cppgc objects. new (data) EmbedderTypeInfo(obj->type(), EmbedderTypeInfo::MemoryMode::kBaseObject);