SERVER-50061 convertFrom() assumes NumberDecimal has machine byte order

6 files changed, 32 insertions, 23 deletions

diff --git a/src/mongo/db/exec/sbe/sbe_key_string_test.cpp b/src/mongo/db/exec/sbe/sbe_key_string_test.cpp
index bdb4879821d..da01af3d6f7 100644
--- a/src/mongo/db/exec/sbe/sbe_key_string_test.cpp
+++ b/src/mongo/db/exec/sbe/sbe_key_string_test.cpp
@@ -195,7 +195,7 @@ TEST(SBEKeyStringTest, KeyComponentInclusion) {
     readKeyStringValueIntoAccessors(
         keyString, KeyString::ALL_ASCENDING, &builder, &accessors, indexKeysToInclude);
 
-    ASSERT(std::make_pair(value::TypeTags::NumberInt64, value::bitcastFrom(12345)) ==
+    ASSERT(std::make_pair(value::TypeTags::NumberInt64, value::bitcastFrom(int64_t{12345})) ==
            accessors[0].getViewOfValue())
         << "Incorrect value from accessor: " << valueDebugString(accessors[0].getViewOfValue());
 
diff --git a/src/mongo/db/exec/sbe/sbe_numeric_convert_test.cpp b/src/mongo/db/exec/sbe/sbe_numeric_convert_test.cpp
index e31effefdce..13ec805c370 100644
--- a/src/mongo/db/exec/sbe/sbe_numeric_convert_test.cpp
+++ b/src/mongo/db/exec/sbe/sbe_numeric_convert_test.cpp
@@ -182,19 +182,19 @@ TEST_F(SBENumericTest, Int64ToInt32) {
 
 TEST_F(SBENumericTest, Int64ToInt64) {
     assertConversion(int64_t{-2147483648},
-                     int32_t{-2147483648},
+                     int64_t{-2147483648},
                      value::TypeTags::NumberInt64,
                      value::TypeTags::NumberInt64);
     assertConversion(
-        int64_t{-10}, int32_t{-10}, value::TypeTags::NumberInt64, value::TypeTags::NumberInt64);
+        int64_t{-10}, int64_t{-10}, value::TypeTags::NumberInt64, value::TypeTags::NumberInt64);
     assertConversion(
-        int64_t{0}, int32_t{0}, value::TypeTags::NumberInt64, value::TypeTags::NumberInt64);
+        int64_t{0}, int64_t{0}, value::TypeTags::NumberInt64, value::TypeTags::NumberInt64);
     assertConversion(
-        int64_t{-0}, int32_t{-0}, value::TypeTags::NumberInt64, value::TypeTags::NumberInt64);
+        int64_t{-0}, int64_t{-0}, value::TypeTags::NumberInt64, value::TypeTags::NumberInt64);
     assertConversion(
-        int64_t{10}, int32_t{10}, value::TypeTags::NumberInt64, value::TypeTags::NumberInt64);
+        int64_t{10}, int64_t{10}, value::TypeTags::NumberInt64, value::TypeTags::NumberInt64);
     assertConversion(int64_t{2147483647},
-                     int32_t{2147483647},
+                     int64_t{2147483647},
                      value::TypeTags::NumberInt64,
                      value::TypeTags::NumberInt64);
 }
diff --git a/src/mongo/db/exec/sbe/values/bson.cpp b/src/mongo/db/exec/sbe/values/bson.cpp
index 43176a63551..a9be90ece32 100644
--- a/src/mongo/db/exec/sbe/values/bson.cpp
+++ b/src/mongo/db/exec/sbe/values/bson.cpp
@@ -106,11 +106,7 @@ std::pair<value::TypeTags, value::Value> convertFrom(bool view,
                 return {value::TypeTags::NumberDecimal, value::bitcastFrom(be)};
             }
 
-            uint64_t low = ConstDataView(be).read<LittleEndian<uint64_t>>();
-            uint64_t high = ConstDataView(be + sizeof(uint64_t)).read<LittleEndian<uint64_t>>();
-            auto dec = Decimal128{Decimal128::Value({low, high})};
-
-            return value::makeCopyDecimal(dec);
+            return value::makeCopyDecimal(value::readDecimal128FromMemory(ConstDataView{be}));
         }
         case BSONType::String: {
             if (view) {
diff --git a/src/mongo/db/exec/sbe/values/value.cpp b/src/mongo/db/exec/sbe/values/value.cpp
index 99ac902b9f9..d2d1c55b3d0 100644
--- a/src/mongo/db/exec/sbe/values/value.cpp
+++ b/src/mongo/db/exec/sbe/values/value.cpp
@@ -55,7 +55,7 @@ std::pair<TypeTags, Value> makeCopyPcreRegex(const pcrecpp::RE& regex) {
 void releaseValue(TypeTags tag, Value val) noexcept {
     switch (tag) {
         case TypeTags::NumberDecimal:
-            delete getDecimalView(val);
+            delete[] getRawPointerView(val);
             break;
         case TypeTags::Array:
             delete getArrayView(val);
diff --git a/src/mongo/db/exec/sbe/values/value.h b/src/mongo/db/exec/sbe/values/value.h
index 4bd929ad7ed..5c4ebe07091 100644
--- a/src/mongo/db/exec/sbe/values/value.h
+++ b/src/mongo/db/exec/sbe/values/value.h
@@ -440,6 +440,12 @@ T readFromMemory(const unsigned char* memory) noexcept {
     return val;
 }
 
+inline Decimal128 readDecimal128FromMemory(const ConstDataView& view) {
+    uint64_t low = view.read<LittleEndian<long long>>();
+    uint64_t high = view.read<LittleEndian<long long>>(sizeof(long long));
+    return Decimal128{Decimal128::Value{low, high}};
+}
+
 template <typename T>
 size_t writeToMemory(unsigned char* memory, const T val) noexcept {
     memcpy(memory, &val, sizeof(T));
@@ -451,6 +457,16 @@ template <typename T>
 Value bitcastFrom(const T in) noexcept {
     static_assert(sizeof(Value) >= sizeof(T));
 
+    // Callers must not try to store a pointer to a Decimal128 object in an sbe::value::Value. Any
+    // Value with the NumberDecimal TypeTag actually stores a pointer to a NumberDecimal as it would
+    // be represented in a BSONElement: a pair of network-ordered (little-endian) uint64_t values.
+    // These bytes are _not_ guaranteed to be the same as the bytes in a Decimal128_t object.
+    //
+    // To get a NumberDecimal value, either call makeCopyDecimal() or store the value in BSON and
+    // use sbe::bson::convertFrom().
+    static_assert(!std::is_same_v<Decimal128, T>);
+    static_assert(!std::is_same_v<Decimal128*, T>);
+
     // Casting from pointer to integer value is OK.
     if constexpr (std::is_pointer_v<T>) {
         return reinterpret_cast<Value>(in);
@@ -468,9 +484,7 @@ T bitcastTo(const Value in) noexcept {
         return reinterpret_cast<T>(in);
     } else if constexpr (std::is_same_v<Decimal128, T>) {
         static_assert(sizeof(Value) == sizeof(T*));
-        T val;
-        memcpy(&val, getRawPointerView(in), sizeof(T));
-        return val;
+        return readDecimal128FromMemory(ConstDataView{getRawPointerView(in)});
     } else {
         static_assert(sizeof(Value) >= sizeof(T));
         T val;
@@ -572,13 +586,12 @@ inline ObjectIdType* getObjectIdView(Value val) noexcept {
     return reinterpret_cast<ObjectIdType*>(val);
 }
 
-inline Decimal128* getDecimalView(Value val) noexcept {
-    return reinterpret_cast<Decimal128*>(val);
-}
-
 inline std::pair<TypeTags, Value> makeCopyDecimal(const Decimal128& inD) {
-    auto o = new Decimal128(inD);
-    return {TypeTags::NumberDecimal, reinterpret_cast<Value>(o)};
+    auto valueBuffer = new char[2 * sizeof(long long)];
+    DataView decimalView(valueBuffer);
+    decimalView.write<LittleEndian<long long>>(inD.getValue().low64, 0);
+    decimalView.write<LittleEndian<long long>>(inD.getValue().high64, sizeof(long long));
+    return {TypeTags::NumberDecimal, reinterpret_cast<Value>(valueBuffer)};
 }
 
 inline KeyString::Value* getKeyStringView(Value val) noexcept {
diff --git a/src/mongo/db/exec/sbe/values/value_builder.h b/src/mongo/db/exec/sbe/values/value_builder.h
index 31e87617e75..08c02ce089e 100644
--- a/src/mongo/db/exec/sbe/values/value_builder.h
+++ b/src/mongo/db/exec/sbe/values/value_builder.h
@@ -146,7 +146,7 @@ public:
 
     void append(const Decimal128& in) {
         appendValueBufferOffset(TypeTags::NumberDecimal);
-        _valueBufferBuilder->appendStruct(in);
+        _valueBufferBuilder->appendNum(in);
     }
 
     void append(long long in) {