Partial fix for retaining Inline scalar in mutable container [#223]

Fleece/Core/Value.cc

@@ -309,6 +309,11 @@ namespace fleece { namespace impl {
     }
 
 
+    bool Value::isMutable() const {
+        return HeapValue::isHeapValue(this);
+    }
+
+
 #pragma mark - VALIDATION:
 
 

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;

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 = 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;
     }

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);

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;
     };
 
 } }

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<MutableArray> 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<MutableArray> 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: