From 3910687b5dd651ff2ec300a9ccf1462c242b4988 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. --- Fleece/Core/Value.cc | 5 +++++ Fleece/Core/Value.hh | 2 +- Fleece/Mutable/HeapValue.cc | 22 +++++++++++++++++----- Fleece/Mutable/HeapValue.hh | 11 ++++++++--- Fleece/Mutable/ValueSlot.hh | 9 ++++++++- Tests/MutableTests.cc | 3 +++ 6 files changed, 42 insertions(+), 10 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..f3fb2827 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,23 @@ 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); + "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..6e6a252f 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) @@ -48,10 +50,13 @@ namespace fleece { namespace impl { static HeapValue* createData(slice s) {return createStr(internal::kBinaryTag, s);} static HeapValue* create(const Value*); - static const Value* asValue(HeapValue *v) FLPURE {return v ? v->asValue() : nullptr;} + 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..6bb3cffb 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);