From d38849edc6e4674cfea589c2ef51c78443a70e1f Mon Sep 17 00:00:00 2001 From: Jens Alfke Date: Thu, 6 Jun 2024 14:49:06 -0700 Subject: [PATCH] Partial fix for retaining Inline scalar in mutable container [#223] This is a bandaid for a nasty architectural bug; see the Github issue. Retaining still doesn't work, but it now throws an exception instead of overwriting out-of-bounds memory. --- Fleece/Core/Value.cc | 5 +++++ Fleece/Core/Value.hh | 2 +- Fleece/Mutable/HeapValue.cc | 19 ++++++++++++++++--- Fleece/Mutable/HeapValue.hh | 9 +++++++-- Fleece/Mutable/ValueSlot.hh | 9 ++++++++- Tests/MutableTests.cc | 24 ++++++++++++++++++++++++ 6 files changed, 61 insertions(+), 7 deletions(-) diff --git a/Fleece/Core/Value.cc b/Fleece/Core/Value.cc index 5ea450b0..04250a7c 100644 --- a/Fleece/Core/Value.cc +++ b/Fleece/Core/Value.cc @@ -309,6 +309,11 @@ namespace fleece { namespace impl { } + bool Value::isMutable() const { + return HeapValue::isHeapValue(this); + } + + #pragma mark - VALIDATION: diff --git a/Fleece/Core/Value.hh b/Fleece/Core/Value.hh index 964ccc77..6ad4006f 100644 --- a/Fleece/Core/Value.hh +++ b/Fleece/Core/Value.hh @@ -142,7 +142,7 @@ namespace fleece { namespace impl { alloc_slice toString() const; /** Returns true if this value is a mutable array or dict. */ - bool isMutable() const FLPURE {return ((size_t)this & 1) != 0;} + bool isMutable() const FLPURE; /** Looks up the SharedKeys from the enclosing Doc (if any.) */ SharedKeys* sharedKeys() const noexcept FLPURE; diff --git a/Fleece/Mutable/HeapValue.cc b/Fleece/Mutable/HeapValue.cc index 43373550..9c5771df 100644 --- a/Fleece/Mutable/HeapValue.cc +++ b/Fleece/Mutable/HeapValue.cc @@ -37,7 +37,6 @@ namespace fleece { namespace impl { namespace internal { HeapValue::HeapValue(tags tag, int tiny) { - _pad = 0xFF; _header = uint8_t((tag << 4) | tiny); } @@ -140,7 +139,7 @@ namespace fleece { namespace impl { namespace internal { if (!isHeapValue(v)) return nullptr; auto ov = (offsetValue*)(size_t(v) & ~1); - assert_postcondition(ov->_pad == 0xFF); + assert_postcondition(ov->_pad == kHeapValuePad); return (HeapValue*)ov; } @@ -159,10 +158,24 @@ namespace fleece { namespace impl { namespace internal { RetainedConst doc = Doc::containing(v); if (_usuallyTrue(doc != nullptr)) (void)fleece::retain(std::move(doc)); - else if (!isHardwiredValue(v)) + else if (!isHardwiredValue(v)) { +#ifdef __LITTLE_ENDIAN__ + if (ValueSlot::isInlineValue(v)) { + // This is an annoying limitation, currently. This Value is contained in a + // ValueSlot in a mutable Array or Dict. We could effectively retain it by + // retaining the container ... but there's not enough information in the slot + // to locate the container. (See issue #223) + // In big-endian we can't even detect this situation because inline values in + // a ValueSlot are even-aligned, indistinguishably from an immutable Value. + FleeceException::_throw(InvalidData, + "Can't retain scalar Value %p that's inline in a " + "mutable container [Fleece #223]", v); + } +#endif FleeceException::_throw(InvalidData, "Can't retain immutable Value %p that's not part of a Doc", v); + } } return v; } diff --git a/Fleece/Mutable/HeapValue.hh b/Fleece/Mutable/HeapValue.hh index cf6270cc..4eb6390f 100644 --- a/Fleece/Mutable/HeapValue.hh +++ b/Fleece/Mutable/HeapValue.hh @@ -20,8 +20,10 @@ namespace fleece { namespace impl { namespace internal { using namespace fleece::impl; + constexpr uint8_t kHeapValuePad = 0xFF; + struct offsetValue { - uint8_t _pad = 0xFF; // Unused byte, to ensure _header is at an odd address + uint8_t _pad = kHeapValuePad; // Unused byte, to ensure _header is at an odd address uint8_t _header; // Value header byte (tag | tiny) // uint8_t _data[0]; // Extra Value data (object is dynamically sized) @@ -51,7 +53,10 @@ namespace fleece { namespace impl { static const Value* asValue(HeapValue *v) FLPURE {return v ? v->asValue() : nullptr;} const Value* asValue() const FLPURE {return (const Value*)&_header;} - static bool isHeapValue(const Value *v) FLPURE {return ((size_t)v & 1) != 0;} + static bool isHeapValue(const Value *v) FLPURE { + return ((size_t)v & 1) != 0 && ((uint8_t*)v)[-1] == kHeapValuePad; + } + static HeapValue* asHeapValue(const Value*); static const Value* retain(const Value *v); diff --git a/Fleece/Mutable/ValueSlot.hh b/Fleece/Mutable/ValueSlot.hh index c20397e3..3d2d1329 100644 --- a/Fleece/Mutable/ValueSlot.hh +++ b/Fleece/Mutable/ValueSlot.hh @@ -64,6 +64,12 @@ namespace fleece { namespace impl { /** Replaces an external value with a copy of itself. */ void copyValue(CopyFlags); +#ifdef __LITTLE_ENDIAN__ + static bool isInlineValue(const Value* v) { + return ((size_t)v & 1) != 0 && ((uint8_t*)v)[-1] == kInlineTag; + } +#endif + protected: friend class internal::HeapArray; friend class internal::HeapDict; @@ -120,7 +126,8 @@ namespace fleece { namespace impl { }; }; - static constexpr uint8_t kInlineTag = 0xFF; + // Value stored in `_tag`. Must have 2 LSB set. Must not be equal to kHeapValuePad. + static constexpr uint8_t kInlineTag = 0x8F; }; } } diff --git a/Tests/MutableTests.cc b/Tests/MutableTests.cc index ca9dc708..9e669651 100644 --- a/Tests/MutableTests.cc +++ b/Tests/MutableTests.cc @@ -121,9 +121,12 @@ namespace fleece { MutableArray::iterator i(ma); for (int n = 0; n < kSize; ++n) { std::cerr << "Item " << n << ": " << (void*)i.value() << "\n"; + INFO("Item " << n << ": " << (void*)i.value()); CHECK(i); CHECK(i.value() != nullptr); CHECK(i.value()->type() == kExpectedTypes[n]); + bool expectMutable = (n == 8 || n == 10 || n == 12 || n >= 15); + CHECK(i.value()->isMutable() == expectMutable); ++i; } CHECK(!i); @@ -283,6 +286,7 @@ namespace fleece { CHECK(copy->get(0)->asArray()->get(0) != a); // it's so deep you can't get under it } + TEST_CASE("MutableArray comparison after resize", "[Mutable]") { // https://github.com/couchbaselabs/fleece/issues/102 Retained ma0 = MutableArray::newArray(); @@ -298,6 +302,26 @@ namespace fleece { } + TEST_CASE("Retain scalar in mutable collection (throws!)", "[Mutable]") { + // Test case for #223, "Can't retain a scalar that's inline in a mutable collection". + Retained ma = MutableArray::newArray(); + ma->append(true); + const Value* scalar = ma->get(0); + CHECK(scalar->type() == valueType::kBoolean); + // #223 causes this to return true, because the scalar in a ValueSlot happens to have an + // odd address like a mutable value, and also the same 0xFF tag byte: + CHECK(!scalar->isMutable()); + // Now trigger the bug. #223 causes `retain` to write 4 bytes starting 5 bytes before + // `scalar`: a buffer overrun that either triggers ASan, corrupts an earlier array item, + // or corrupts the heap :-o + // + // (The expression below should be `retain(scalar)`, but that function is `noexcept` so + // the exception would terminate the process. Workaround is to "inline" `retain` by + // directly calling `->_retain()`.) + REQUIRE_THROWS_AS(scalar->_retain(), FleeceException); + } + + #pragma mark - MUTABLE DICT: