diff --git a/CHANGELOG.md b/CHANGELOG.md index 13937981..d95d6bc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,36 @@ HEAD ---- * Fix support for NUL characters in `deserializeJson()` +* Make `ElementProxy` and `MemberProxy` non-copyable + +> ### BREAKING CHANGES +> +> In previous versions, `MemberProxy` (the class returned by `operator[]`) could lead to dangling pointers when used with a temporary string. +> To prevent this issue, `MemberProxy` and `ElementProxy` are now non-copyable. +> +> Your code is likely to be affected if you use `auto` to store the result of `operator[]`. For example, the following line won't compile anymore: +> +> ```cpp +> auto value = doc["key"]; +> ``` +> +> To fix the issue, you must append either `.as()` or `.to()`, depending on the situation. +> +> For example, if you are extracting values from a JSON document, you should update like this: +> +> ```diff +> - auto config = doc["config"]; +> + auto config = doc["config"].as(); +> const char* name = config["name"]; +> ``` +> +> However, if you are building a JSON document, you should update like this: +> +> ```diff +> - auto config = doc["config"]; +> + auto config = doc["config"].to(); +> config["name"] = "ArduinoJson"; +> ``` v7.2.1 (2024-11-15) ------ diff --git a/extras/tests/Deprecated/containsKey.cpp b/extras/tests/Deprecated/containsKey.cpp index 1b508975..846e98fb 100644 --- a/extras/tests/Deprecated/containsKey.cpp +++ b/extras/tests/Deprecated/containsKey.cpp @@ -67,7 +67,7 @@ TEST_CASE("JsonDocument::containsKey()") { TEST_CASE("MemberProxy::containsKey()") { JsonDocument doc; - auto mp = doc["hello"]; + const auto& mp = doc["hello"]; SECTION("containsKey(const char*)") { mp["key"] = "value"; diff --git a/extras/tests/JsonDocument/ElementProxy.cpp b/extras/tests/JsonDocument/ElementProxy.cpp index 7faf0e47..7de4d6ab 100644 --- a/extras/tests/JsonDocument/ElementProxy.cpp +++ b/extras/tests/JsonDocument/ElementProxy.cpp @@ -12,7 +12,7 @@ using ElementProxy = ArduinoJson::detail::ElementProxy; TEST_CASE("ElementProxy::add()") { JsonDocument doc; doc.add(); - ElementProxy ep = doc[0]; + const ElementProxy& ep = doc[0]; SECTION("add(int)") { ep.add(42); @@ -50,7 +50,7 @@ TEST_CASE("ElementProxy::add()") { TEST_CASE("ElementProxy::clear()") { JsonDocument doc; doc.add(); - ElementProxy ep = doc[0]; + const ElementProxy& ep = doc[0]; SECTION("size goes back to zero") { ep.add(42); @@ -110,7 +110,7 @@ TEST_CASE("ElementProxy::operator==()") { TEST_CASE("ElementProxy::remove()") { JsonDocument doc; doc.add(); - ElementProxy ep = doc[0]; + const ElementProxy& ep = doc[0]; SECTION("remove(int)") { ep.add(1); @@ -157,7 +157,7 @@ TEST_CASE("ElementProxy::remove()") { TEST_CASE("ElementProxy::set()") { JsonDocument doc; - ElementProxy ep = doc[0]; + const ElementProxy& ep = doc[0]; SECTION("set(int)") { ep.set(42); @@ -195,7 +195,7 @@ TEST_CASE("ElementProxy::set()") { TEST_CASE("ElementProxy::size()") { JsonDocument doc; doc.add(); - ElementProxy ep = doc[0]; + const ElementProxy& ep = doc[0]; SECTION("returns 0") { REQUIRE(ep.size() == 0); @@ -216,7 +216,7 @@ TEST_CASE("ElementProxy::size()") { TEST_CASE("ElementProxy::operator[]") { JsonDocument doc; - ElementProxy ep = doc[1]; + const ElementProxy& ep = doc[1]; SECTION("set member") { ep["world"] = 42; @@ -247,7 +247,7 @@ TEST_CASE("ElementProxy cast to JsonVariantConst") { JsonDocument doc; doc[0] = "world"; - const ElementProxy ep = doc[0]; + const ElementProxy& ep = doc[0]; JsonVariantConst var = ep; @@ -258,7 +258,7 @@ TEST_CASE("ElementProxy cast to JsonVariant") { JsonDocument doc; doc[0] = "world"; - ElementProxy ep = doc[0]; + const ElementProxy& ep = doc[0]; JsonVariant var = ep; diff --git a/extras/tests/JsonDocument/MemberProxy.cpp b/extras/tests/JsonDocument/MemberProxy.cpp index 96c24a95..79f314c0 100644 --- a/extras/tests/JsonDocument/MemberProxy.cpp +++ b/extras/tests/JsonDocument/MemberProxy.cpp @@ -16,7 +16,7 @@ using ArduinoJson::detail::sizeofObject; TEST_CASE("MemberProxy::add()") { JsonDocument doc; - auto mp = doc["hello"]; + const auto& mp = doc["hello"]; SECTION("add(int)") { mp.add(42); @@ -45,7 +45,7 @@ TEST_CASE("MemberProxy::add()") { TEST_CASE("MemberProxy::clear()") { JsonDocument doc; - auto mp = doc["hello"]; + const auto& mp = doc["hello"]; SECTION("size goes back to zero") { mp.add(42); @@ -136,7 +136,7 @@ TEST_CASE("MemberProxy::operator|()") { TEST_CASE("MemberProxy::remove()") { JsonDocument doc; - auto mp = doc["hello"]; + const auto& mp = doc["hello"]; SECTION("remove(int)") { mp.add(1); @@ -183,7 +183,7 @@ TEST_CASE("MemberProxy::remove()") { TEST_CASE("MemberProxy::set()") { JsonDocument doc; - auto mp = doc["hello"]; + const auto& mp = doc["hello"]; SECTION("set(int)") { mp.set(42); @@ -220,7 +220,7 @@ TEST_CASE("MemberProxy::set()") { TEST_CASE("MemberProxy::size()") { JsonDocument doc; - auto mp = doc["hello"]; + const auto& mp = doc["hello"]; SECTION("returns 0") { REQUIRE(mp.size() == 0); @@ -243,7 +243,7 @@ TEST_CASE("MemberProxy::size()") { TEST_CASE("MemberProxy::operator[]") { JsonDocument doc; - auto mp = doc["hello"]; + const auto& mp = doc["hello"]; SECTION("set member") { mp["world"] = 42; @@ -262,7 +262,7 @@ TEST_CASE("MemberProxy cast to JsonVariantConst") { JsonDocument doc; doc["hello"] = "world"; - const auto mp = doc["hello"]; + const auto& mp = doc["hello"]; JsonVariantConst var = mp; @@ -273,7 +273,7 @@ TEST_CASE("MemberProxy cast to JsonVariant") { JsonDocument doc; doc["hello"] = "world"; - auto mp = doc["hello"]; + const auto& mp = doc["hello"]; JsonVariant var = mp; diff --git a/extras/tests/JsonDocument/issue1120.cpp b/extras/tests/JsonDocument/issue1120.cpp index 07ad9324..aa3132f5 100644 --- a/extras/tests/JsonDocument/issue1120.cpp +++ b/extras/tests/JsonDocument/issue1120.cpp @@ -12,49 +12,41 @@ TEST_CASE("Issue #1120") { SECTION("MemberProxy::isNull()") { SECTION("returns false") { - auto value = doc["contents"_s]; - CHECK(value.isNull() == false); + CHECK(doc["contents"_s].isNull() == false); } SECTION("returns true") { - auto value = doc["zontents"_s]; - CHECK(value.isNull() == true); + CHECK(doc["zontents"_s].isNull() == true); } } SECTION("ElementProxy >::isNull()") { SECTION("returns false") { // Issue #1120 - auto value = doc["contents"][1]; - CHECK(value.isNull() == false); + CHECK(doc["contents"][1].isNull() == false); } SECTION("returns true") { - auto value = doc["contents"][2]; - CHECK(value.isNull() == true); + CHECK(doc["contents"][2].isNull() == true); } } SECTION("MemberProxy, const char*>::isNull()") { SECTION("returns false") { - auto value = doc["contents"][1]["module"]; - CHECK(value.isNull() == false); + CHECK(doc["contents"][1]["module"].isNull() == false); } SECTION("returns true") { - auto value = doc["contents"][1]["zodule"]; - CHECK(value.isNull() == true); + CHECK(doc["contents"][1]["zodule"].isNull() == true); } } SECTION("MemberProxy, std::string>::isNull()") { SECTION("returns false") { - auto value = doc["contents"][1]["module"_s]; - CHECK(value.isNull() == false); + CHECK(doc["contents"][1]["module"_s].isNull() == false); } SECTION("returns true") { - auto value = doc["contents"][1]["zodule"_s]; - CHECK(value.isNull() == true); + CHECK(doc["contents"][1]["zodule"_s].isNull() == true); } } } diff --git a/src/ArduinoJson/Array/ElementProxy.hpp b/src/ArduinoJson/Array/ElementProxy.hpp index f36b68ea..06994a93 100644 --- a/src/ArduinoJson/Array/ElementProxy.hpp +++ b/src/ArduinoJson/Array/ElementProxy.hpp @@ -15,13 +15,18 @@ class ElementProxy : public VariantRefBase>, public VariantOperators> { friend class VariantAttorney; + friend class VariantRefBase>; + + template + friend class MemberProxy; + + template + friend class ElementProxy; + public: ElementProxy(TUpstream upstream, size_t index) : upstream_(upstream), index_(index) {} - ElementProxy(const ElementProxy& src) - : upstream_(src.upstream_), index_(src.index_) {} - ElementProxy& operator=(const ElementProxy& src) { this->set(src); return *this; @@ -40,6 +45,11 @@ class ElementProxy : public VariantRefBase>, } private: + // clang-format off + ElementProxy(const ElementProxy& src) // Error here? See https://arduinojson.org/v7/proxy-non-copyable/ + : upstream_(src.upstream_), index_(src.index_) {} + // clang-format on + ResourceManager* getResourceManager() const { return VariantAttorney::getResourceManager(upstream_); } diff --git a/src/ArduinoJson/Array/JsonArray.hpp b/src/ArduinoJson/Array/JsonArray.hpp index 0e3ea2ee..e9cbca71 100644 --- a/src/ArduinoJson/Array/JsonArray.hpp +++ b/src/ArduinoJson/Array/JsonArray.hpp @@ -116,7 +116,7 @@ class JsonArray : public detail::VariantOperators { // https://arduinojson.org/v7/api/jsonarray/remove/ template detail::enable_if_t::value> remove( - TVariant variant) const { + const TVariant& variant) const { if (variant.template is()) remove(variant.template as()); } @@ -143,7 +143,7 @@ class JsonArray : public detail::VariantOperators { detail::ElementProxy> operator[](const TVariant& variant) const { if (variant.template is()) - return operator[](variant.template as()); + return {*this, variant.template as()}; else return {*this, size_t(-1)}; } diff --git a/src/ArduinoJson/Object/MemberProxy.hpp b/src/ArduinoJson/Object/MemberProxy.hpp index 8c4313af..2d297585 100644 --- a/src/ArduinoJson/Object/MemberProxy.hpp +++ b/src/ArduinoJson/Object/MemberProxy.hpp @@ -16,13 +16,18 @@ class MemberProxy public VariantOperators> { friend class VariantAttorney; + friend class VariantRefBase>; + + template + friend class MemberProxy; + + template + friend class ElementProxy; + public: MemberProxy(TUpstream upstream, AdaptedString key) : upstream_(upstream), key_(key) {} - MemberProxy(const MemberProxy& src) - : upstream_(src.upstream_), key_(src.key_) {} - MemberProxy& operator=(const MemberProxy& src) { this->set(src); return *this; @@ -41,6 +46,11 @@ class MemberProxy } private: + // clang-format off + MemberProxy(const MemberProxy& src) // Error here? See https://arduinojson.org/v7/proxy-non-copyable/ + : upstream_(src.upstream_), key_(src.key_) {} + // clang-format on + ResourceManager* getResourceManager() const { return VariantAttorney::getResourceManager(upstream_); } diff --git a/src/ArduinoJson/Variant/VariantOperators.hpp b/src/ArduinoJson/Variant/VariantOperators.hpp index b06d46f6..08e5bbd4 100644 --- a/src/ArduinoJson/Variant/VariantOperators.hpp +++ b/src/ArduinoJson/Variant/VariantOperators.hpp @@ -51,7 +51,7 @@ struct VariantOperators : VariantOperatorTag { // JsonVariant operator|(JsonVariant, JsonVariant) template friend enable_if_t::value, JsonVariantConst> operator|( - const TVariant& variant, T defaultValue) { + const TVariant& variant, const T& defaultValue) { if (variant) return variant; else @@ -60,38 +60,38 @@ struct VariantOperators : VariantOperatorTag { // value == TVariant template - friend bool operator==(T* lhs, TVariant rhs) { + friend bool operator==(T* lhs, const TVariant& rhs) { return compare(rhs, lhs) == COMPARE_RESULT_EQUAL; } template - friend bool operator==(const T& lhs, TVariant rhs) { + friend bool operator==(const T& lhs, const TVariant& rhs) { return compare(rhs, lhs) == COMPARE_RESULT_EQUAL; } // TVariant == value template - friend bool operator==(TVariant lhs, T* rhs) { + friend bool operator==(const TVariant& lhs, T* rhs) { return compare(lhs, rhs) == COMPARE_RESULT_EQUAL; } template friend enable_if_t::value, bool> - operator==(TVariant lhs, const T& rhs) { + operator==(const TVariant& lhs, const T& rhs) { return compare(lhs, rhs) == COMPARE_RESULT_EQUAL; } // value != TVariant template - friend bool operator!=(T* lhs, TVariant rhs) { + friend bool operator!=(T* lhs, const TVariant& rhs) { return compare(rhs, lhs) != COMPARE_RESULT_EQUAL; } template - friend bool operator!=(const T& lhs, TVariant rhs) { + friend bool operator!=(const T& lhs, const TVariant& rhs) { return compare(rhs, lhs) != COMPARE_RESULT_EQUAL; } // TVariant != value template - friend bool operator!=(TVariant lhs, T* rhs) { + friend bool operator!=(const TVariant& lhs, T* rhs) { return compare(lhs, rhs) != COMPARE_RESULT_EQUAL; } template @@ -102,17 +102,17 @@ struct VariantOperators : VariantOperatorTag { // value < TVariant template - friend bool operator<(T* lhs, TVariant rhs) { + friend bool operator<(T* lhs, const TVariant& rhs) { return compare(rhs, lhs) == COMPARE_RESULT_GREATER; } template - friend bool operator<(const T& lhs, TVariant rhs) { + friend bool operator<(const T& lhs, const TVariant& rhs) { return compare(rhs, lhs) == COMPARE_RESULT_GREATER; } // TVariant < value template - friend bool operator<(TVariant lhs, T* rhs) { + friend bool operator<(const TVariant& lhs, T* rhs) { return compare(lhs, rhs) == COMPARE_RESULT_LESS; } template @@ -123,17 +123,17 @@ struct VariantOperators : VariantOperatorTag { // value <= TVariant template - friend bool operator<=(T* lhs, TVariant rhs) { + friend bool operator<=(T* lhs, const TVariant& rhs) { return (compare(rhs, lhs) & COMPARE_RESULT_GREATER_OR_EQUAL) != 0; } template - friend bool operator<=(const T& lhs, TVariant rhs) { + friend bool operator<=(const T& lhs, const TVariant& rhs) { return (compare(rhs, lhs) & COMPARE_RESULT_GREATER_OR_EQUAL) != 0; } // TVariant <= value template - friend bool operator<=(TVariant lhs, T* rhs) { + friend bool operator<=(const TVariant& lhs, T* rhs) { return (compare(lhs, rhs) & COMPARE_RESULT_LESS_OR_EQUAL) != 0; } template @@ -144,17 +144,17 @@ struct VariantOperators : VariantOperatorTag { // value > TVariant template - friend bool operator>(T* lhs, TVariant rhs) { + friend bool operator>(T* lhs, const TVariant& rhs) { return compare(rhs, lhs) == COMPARE_RESULT_LESS; } template - friend bool operator>(const T& lhs, TVariant rhs) { + friend bool operator>(const T& lhs, const TVariant& rhs) { return compare(rhs, lhs) == COMPARE_RESULT_LESS; } // TVariant > value template - friend bool operator>(TVariant lhs, T* rhs) { + friend bool operator>(const TVariant& lhs, T* rhs) { return compare(lhs, rhs) == COMPARE_RESULT_GREATER; } template @@ -165,22 +165,22 @@ struct VariantOperators : VariantOperatorTag { // value >= TVariant template - friend bool operator>=(T* lhs, TVariant rhs) { + friend bool operator>=(T* lhs, const TVariant& rhs) { return (compare(rhs, lhs) & COMPARE_RESULT_LESS_OR_EQUAL) != 0; } template - friend bool operator>=(const T& lhs, TVariant rhs) { + friend bool operator>=(const T& lhs, const TVariant& rhs) { return (compare(rhs, lhs) & COMPARE_RESULT_LESS_OR_EQUAL) != 0; } // TVariant >= value template - friend bool operator>=(TVariant lhs, T* rhs) { + friend bool operator>=(const TVariant& lhs, T* rhs) { return (compare(lhs, rhs) & COMPARE_RESULT_GREATER_OR_EQUAL) != 0; } template friend enable_if_t::value, bool> - operator>=(TVariant lhs, const T& rhs) { + operator>=(const TVariant& lhs, const T& rhs) { return (compare(lhs, rhs) & COMPARE_RESULT_GREATER_OR_EQUAL) != 0; } }; diff --git a/src/ArduinoJson/Variant/VariantRefBaseImpl.hpp b/src/ArduinoJson/Variant/VariantRefBaseImpl.hpp index 5838f9f3..1a0685f3 100644 --- a/src/ArduinoJson/Variant/VariantRefBaseImpl.hpp +++ b/src/ArduinoJson/Variant/VariantRefBaseImpl.hpp @@ -119,7 +119,7 @@ inline bool VariantRefBase::is() const { template inline ElementProxy VariantRefBase::operator[]( size_t index) const { - return ElementProxy(derived(), index); + return {derived(), index}; } template