From 3c837ec5b76acc43ccf064f5c2e299bd31689f01 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 9 Sep 2026 17:46:37 +0800 Subject: [PATCH] src: avoid union type-punning in trace values Replace TraceValueUnion with memcpy-based object representation copying to avoid reading inactive union members. Generalize integral values to support all integral and enum types. This follows the equivalent V8 fix in CL 1886918. Refs: https://chromium-review.googlesource.com/c/v8/v8/+/1886918 Fixes: https://github.com/nodejs/node/issues/57033 Signed-off-by: XadillaX --- src/tracing/trace_event_legacy_inl.h | 75 +++++++++++----------------- test/cctest/test_trace_event.cc | 69 +++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 45 deletions(-) create mode 100644 test/cctest/test_trace_event.cc diff --git a/src/tracing/trace_event_legacy_inl.h b/src/tracing/trace_event_legacy_inl.h index 9030473afe96..d9f9a9e13a62 100644 --- a/src/tracing/trace_event_legacy_inl.h +++ b/src/tracing/trace_event_legacy_inl.h @@ -9,13 +9,15 @@ #error Perfetto is enabled. #endif +#include #include +#include +#include -#include "v8-platform.h" #include "tracing/agent_legacy.h" #include "tracing/trace_event_helper.h" #include "tracing/trace_event_legacy.h" -#include +#include "v8-platform.h" // This header file defines implementation details of how the trace macros in // trace_event_common.h collect and store trace events. Anything not @@ -426,16 +428,6 @@ class TraceID { uint64_t raw_id_; }; -// Simple union to store various types as uint64_t. -union TraceValueUnion { - bool as_bool; - uint64_t as_uint; - int64_t as_int; - double as_double; - const void* as_pointer; - const char* as_string; -}; - // Simple container for const char* that should be copied instead of retained. class TraceStringWithCopy { public: @@ -517,45 +509,38 @@ static V8_INLINE void AddMetadataEventImpl( // Define SetTraceValue for each allowed type. It stores the type and // value in the return arguments. This allows this API to avoid declaring any // structures so that it is portable to third_party libraries. -#define INTERNAL_DECLARE_SET_TRACE_VALUE(actual_type, union_member, \ - value_type_id) \ - static inline void SetTraceValue(actual_type arg, unsigned char* type, \ - uint64_t* value) { \ - TraceValueUnion type_value; \ - type_value.union_member = arg; \ - *type = value_type_id; \ - *value = type_value.as_uint; \ - } -// Simpler form for int types that can be safely casted. -#define INTERNAL_DECLARE_SET_TRACE_VALUE_INT(actual_type, value_type_id) \ - static inline void SetTraceValue(actual_type arg, unsigned char* type, \ - uint64_t* value) { \ - *type = value_type_id; \ - *value = static_cast(arg); \ +// This follows V8's implementation, which replaced union type-punning with +// memcpy in https://crrev.com/c/1886918. +template +static inline std::enable_if_t || std::is_enum_v> +SetTraceValue(T arg, unsigned char* type, uint64_t* value) { + *type = std::is_same_v ? TRACE_VALUE_TYPE_BOOL + : std::is_signed_v ? TRACE_VALUE_TYPE_INT + : TRACE_VALUE_TYPE_UINT; + *value = static_cast(arg); +} + +#define INTERNAL_DECLARE_SET_TRACE_VALUE(actual_type, value_type_id) \ + static inline void SetTraceValue( \ + actual_type arg, unsigned char* type, uint64_t* value) { \ + *type = value_type_id; \ + *value = 0; \ + static_assert(sizeof(arg) <= sizeof(*value)); \ + std::memcpy(value, &arg, sizeof(arg)); \ } -INTERNAL_DECLARE_SET_TRACE_VALUE_INT(uint64_t, TRACE_VALUE_TYPE_UINT) -INTERNAL_DECLARE_SET_TRACE_VALUE_INT(unsigned int, TRACE_VALUE_TYPE_UINT) -INTERNAL_DECLARE_SET_TRACE_VALUE_INT(uint16_t, TRACE_VALUE_TYPE_UINT) -INTERNAL_DECLARE_SET_TRACE_VALUE_INT(unsigned char, TRACE_VALUE_TYPE_UINT) -INTERNAL_DECLARE_SET_TRACE_VALUE_INT(int64_t, TRACE_VALUE_TYPE_INT) -INTERNAL_DECLARE_SET_TRACE_VALUE_INT(int, TRACE_VALUE_TYPE_INT) -INTERNAL_DECLARE_SET_TRACE_VALUE_INT(int16_t, TRACE_VALUE_TYPE_INT) -INTERNAL_DECLARE_SET_TRACE_VALUE_INT(signed char, TRACE_VALUE_TYPE_INT) -INTERNAL_DECLARE_SET_TRACE_VALUE(bool, as_bool, TRACE_VALUE_TYPE_BOOL) -INTERNAL_DECLARE_SET_TRACE_VALUE(double, as_double, TRACE_VALUE_TYPE_DOUBLE) -INTERNAL_DECLARE_SET_TRACE_VALUE(const void*, as_pointer, - TRACE_VALUE_TYPE_POINTER) -INTERNAL_DECLARE_SET_TRACE_VALUE(const char*, as_string, - TRACE_VALUE_TYPE_STRING) -INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWithCopy&, as_string, +INTERNAL_DECLARE_SET_TRACE_VALUE(double, TRACE_VALUE_TYPE_DOUBLE) +INTERNAL_DECLARE_SET_TRACE_VALUE(const void*, TRACE_VALUE_TYPE_POINTER) +INTERNAL_DECLARE_SET_TRACE_VALUE(const char*, TRACE_VALUE_TYPE_STRING) +INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWithCopy&, TRACE_VALUE_TYPE_COPY_STRING) #undef INTERNAL_DECLARE_SET_TRACE_VALUE -#undef INTERNAL_DECLARE_SET_TRACE_VALUE_INT -static inline void SetTraceValue(v8::ConvertableToTraceFormat* convertable_value, - unsigned char* type, uint64_t* value) { +static inline void SetTraceValue( + v8::ConvertableToTraceFormat* convertable_value, + unsigned char* type, + uint64_t* value) { *type = TRACE_VALUE_TYPE_CONVERTABLE; *value = static_cast(reinterpret_cast(convertable_value)); } diff --git a/test/cctest/test_trace_event.cc b/test/cctest/test_trace_event.cc new file mode 100644 index 000000000000..900ab8244730 --- /dev/null +++ b/test/cctest/test_trace_event.cc @@ -0,0 +1,69 @@ +#include "gtest/gtest.h" + +#ifndef V8_USE_PERFETTO + +#include +#include +#include + +#include "tracing/trace_event.h" + +namespace { + +template +T DecodeTraceValue(uint64_t value) { + static_assert(std::is_trivially_copyable_v); + T result{}; + std::memcpy(&result, &value, sizeof(result)); + return result; +} + +enum class TestEnum : uint16_t { + kValue = 42, +}; + +TEST(TraceEvent, SetIntegralTraceValue) { + unsigned char type; + uint64_t value; + + node::tracing::SetTraceValue(false, &type, &value); + EXPECT_EQ(TRACE_VALUE_TYPE_BOOL, type); + EXPECT_EQ(uint64_t{0}, value); + + node::tracing::SetTraceValue(-42, &type, &value); + EXPECT_EQ(TRACE_VALUE_TYPE_INT, type); + EXPECT_EQ(static_cast(-42), value); + + node::tracing::SetTraceValue(TestEnum::kValue, &type, &value); + EXPECT_EQ(TRACE_VALUE_TYPE_UINT, type); + EXPECT_EQ(uint64_t{42}, value); +} + +TEST(TraceEvent, SetNonIntegralTraceValue) { + unsigned char type; + uint64_t value; + + const double number = 1.25; + node::tracing::SetTraceValue(number, &type, &value); + EXPECT_EQ(TRACE_VALUE_TYPE_DOUBLE, type); + EXPECT_EQ(number, DecodeTraceValue(value)); + + const void* pointer = &value; + node::tracing::SetTraceValue(pointer, &type, &value); + EXPECT_EQ(TRACE_VALUE_TYPE_POINTER, type); + EXPECT_EQ(pointer, DecodeTraceValue(value)); + + const char* string = "trace value"; + node::tracing::SetTraceValue(string, &type, &value); + EXPECT_EQ(TRACE_VALUE_TYPE_STRING, type); + EXPECT_EQ(string, DecodeTraceValue(value)); + + const node::tracing::TraceStringWithCopy copied_string(string); + node::tracing::SetTraceValue(copied_string, &type, &value); + EXPECT_EQ(TRACE_VALUE_TYPE_COPY_STRING, type); + EXPECT_EQ(string, DecodeTraceValue(value)); +} + +} // namespace + +#endif // V8_USE_PERFETTO