From 56bf24e1ec76ef331eff2e4c8c57f366f24c8957 Mon Sep 17 00:00:00 2001 From: Benoit Blanchon Date: Mon, 18 Feb 2019 16:04:51 +0100 Subject: [PATCH] Fixed `JsonVariant::isNull()` not returning `true` after `set((char*)0)` --- CHANGELOG.md | 2 + src/ArduinoJson/Json/JsonDeserializer.hpp | 15 +++---- .../MsgPack/MsgPackDeserializer.hpp | 15 +++---- src/ArduinoJson/Polyfills/gsl/not_null.hpp | 33 ++++++++++++++ .../Strings/ConstRamStringAdapter.hpp | 1 + .../Strings/FlashStringAdapter.hpp | 1 + .../Strings/SizedFlashStringAdapter.hpp | 2 +- .../Strings/SizedRamStringAdapter.hpp | 2 +- src/ArduinoJson/Variant/SlotFunctions.hpp | 6 +-- src/ArduinoJson/Variant/VariantData.hpp | 37 ++++++++++------ src/ArduinoJson/Variant/VariantSlot.hpp | 9 ++-- .../deserializeJsonString.cpp | 8 ++++ test/JsonVariant/isnull.cpp | 43 +++++++++++++++---- 13 files changed, 127 insertions(+), 47 deletions(-) create mode 100644 src/ArduinoJson/Polyfills/gsl/not_null.hpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cee9ef1..0b8198cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ HEAD * Renamed `JsonArray::add()` (without arg) to `addElement()` * Renamed `JsonObject::get()` to `getMember()` * Renamed `JsonObject::getOrCreate()` to `getOrAddMember()` +* Fixed `JsonVariant::isNull()` not returning `true` after `set((char*)0)` +* Fixed segfault after `variant.set(serialized((char*)0))` v6.8.0-beta (2019-01-30) ----------- diff --git a/src/ArduinoJson/Json/JsonDeserializer.hpp b/src/ArduinoJson/Json/JsonDeserializer.hpp index 2ca85d1d..f4f749ad 100644 --- a/src/ArduinoJson/Json/JsonDeserializer.hpp +++ b/src/ArduinoJson/Json/JsonDeserializer.hpp @@ -19,7 +19,6 @@ template class JsonDeserializer { typedef typename remove_reference::type::StringBuilder StringBuilder; - typedef const char *StringType; public: JsonDeserializer(MemoryPool &pool, TReader reader, @@ -124,10 +123,10 @@ class JsonDeserializer { if (!slot) return DeserializationError::NoMemory; // Parse key - StringType key; + const char *key; err = parseKey(key); if (err) return err; - slot->setOwnedKey(key); + slot->setOwnedKey(make_not_null(key)); // Skip spaces err = skipSpacesAndComments(); @@ -162,7 +161,7 @@ class JsonDeserializer { } } - DeserializationError parseKey(StringType &key) { + DeserializationError parseKey(const char *&key) { if (isQuote(current())) { return parseQuotedString(key); } else { @@ -171,14 +170,14 @@ class JsonDeserializer { } DeserializationError parseStringValue(VariantData &variant) { - StringType value; + const char *value; DeserializationError err = parseQuotedString(value); if (err) return err; - variant.setOwnedString(value); + variant.setOwnedString(make_not_null(value)); return DeserializationError::Ok; } - DeserializationError parseQuotedString(StringType &result) { + DeserializationError parseQuotedString(const char *&result) { StringBuilder builder = _stringStorage.startString(); const char stopChar = current(); @@ -219,7 +218,7 @@ class JsonDeserializer { return DeserializationError::Ok; } - DeserializationError parseNonQuotedString(StringType &result) { + DeserializationError parseNonQuotedString(const char *&result) { StringBuilder builder = _stringStorage.startString(); char c = current(); diff --git a/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp b/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp index 03525143..9244ecb2 100644 --- a/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp +++ b/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp @@ -17,7 +17,6 @@ template class MsgPackDeserializer { typedef typename remove_reference::type::StringBuilder StringBuilder; - typedef const char *StringType; public: MsgPackDeserializer(MemoryPool &pool, TReader reader, @@ -227,20 +226,20 @@ class MsgPackDeserializer { } template - DeserializationError readString(StringType &str) { + DeserializationError readString(const char *&str) { T size; if (!readInteger(size)) return DeserializationError::IncompleteInput; return readString(str, size); } DeserializationError readString(VariantData &variant, size_t n) { - StringType s; + const char *s; DeserializationError err = readString(s, n); - if (!err) variant.setOwnedString(s); + if (!err) variant.setOwnedString(make_not_null(s)); return err; } - DeserializationError readString(StringType &result, size_t n) { + DeserializationError readString(const char *&result, size_t n) { StringBuilder builder = _stringStorage.startString(); for (; n; --n) { uint8_t c; @@ -287,10 +286,10 @@ class MsgPackDeserializer { VariantSlot *slot = object.addSlot(_pool); if (!slot) return DeserializationError::NoMemory; - StringType key; + const char *key; DeserializationError err = parseKey(key); if (err) return err; - slot->setOwnedKey(key); + slot->setOwnedKey(make_not_null(key)); err = parse(*slot->data()); if (err) return err; @@ -299,7 +298,7 @@ class MsgPackDeserializer { return DeserializationError::Ok; } - DeserializationError parseKey(StringType &key) { + DeserializationError parseKey(const char *&key) { uint8_t code; if (!readByte(code)) return DeserializationError::IncompleteInput; diff --git a/src/ArduinoJson/Polyfills/gsl/not_null.hpp b/src/ArduinoJson/Polyfills/gsl/not_null.hpp new file mode 100644 index 00000000..baa01207 --- /dev/null +++ b/src/ArduinoJson/Polyfills/gsl/not_null.hpp @@ -0,0 +1,33 @@ +// ArduinoJson - arduinojson.org +// Copyright Benoit Blanchon 2014-2019 +// MIT License + +#pragma once + +#include "../assert.hpp" + +namespace ARDUINOJSON_NAMESPACE { + +template +class not_null { + public: + explicit not_null(T ptr) : _ptr(ptr) { + ARDUINOJSON_ASSERT(ptr != NULL); + } + + T get() const { + ARDUINOJSON_ASSERT(_ptr != NULL); + return _ptr; + } + + private: + T _ptr; +}; + +template +not_null make_not_null(T ptr) { + ARDUINOJSON_ASSERT(ptr != NULL); + return not_null(ptr); +} + +} // namespace ARDUINOJSON_NAMESPACE diff --git a/src/ArduinoJson/Strings/ConstRamStringAdapter.hpp b/src/ArduinoJson/Strings/ConstRamStringAdapter.hpp index 75312f42..c8736905 100644 --- a/src/ArduinoJson/Strings/ConstRamStringAdapter.hpp +++ b/src/ArduinoJson/Strings/ConstRamStringAdapter.hpp @@ -29,6 +29,7 @@ class ConstRamStringAdapter { } size_t size() const { + if (!_str) return 0; return strlen(_str); } diff --git a/src/ArduinoJson/Strings/FlashStringAdapter.hpp b/src/ArduinoJson/Strings/FlashStringAdapter.hpp index 99fa4f19..79d26ef8 100644 --- a/src/ArduinoJson/Strings/FlashStringAdapter.hpp +++ b/src/ArduinoJson/Strings/FlashStringAdapter.hpp @@ -33,6 +33,7 @@ class FlashStringAdapter { } size_t size() const { + if (!_str) return 0; return strlen_P(reinterpret_cast(_str)); } diff --git a/src/ArduinoJson/Strings/SizedFlashStringAdapter.hpp b/src/ArduinoJson/Strings/SizedFlashStringAdapter.hpp index 515dcb51..f31b1e3c 100644 --- a/src/ArduinoJson/Strings/SizedFlashStringAdapter.hpp +++ b/src/ArduinoJson/Strings/SizedFlashStringAdapter.hpp @@ -29,7 +29,7 @@ class SizedFlashStringAdapter { } size_t size() const { - return strlen_P(reinterpret_cast(_str)); + return _size; } bool isStatic() const { diff --git a/src/ArduinoJson/Strings/SizedRamStringAdapter.hpp b/src/ArduinoJson/Strings/SizedRamStringAdapter.hpp index 119dfdb1..1333b94e 100644 --- a/src/ArduinoJson/Strings/SizedRamStringAdapter.hpp +++ b/src/ArduinoJson/Strings/SizedRamStringAdapter.hpp @@ -30,7 +30,7 @@ class SizedRamStringAdapter { } size_t size() const { - return strlen(reinterpret_cast(_str)); + return _size; } bool isStatic() const { diff --git a/src/ArduinoJson/Variant/SlotFunctions.hpp b/src/ArduinoJson/Variant/SlotFunctions.hpp index fde0330e..41e0ead9 100644 --- a/src/ArduinoJson/Variant/SlotFunctions.hpp +++ b/src/ArduinoJson/Variant/SlotFunctions.hpp @@ -15,11 +15,11 @@ template inline bool slotSetKey(VariantSlot* var, TAdaptedString key, MemoryPool* pool) { if (!var) return false; if (key.isStatic()) { - var->setLinkedKey(key.data()); + var->setLinkedKey(make_not_null(key.data())); } else { - char* dup = key.save(pool); + const char* dup = key.save(pool); if (!dup) return false; - var->setOwnedKey(dup); + var->setOwnedKey(make_not_null(dup)); } return true; } diff --git a/src/ArduinoJson/Variant/VariantData.hpp b/src/ArduinoJson/Variant/VariantData.hpp index 419f806e..da3c6ac8 100644 --- a/src/ArduinoJson/Variant/VariantData.hpp +++ b/src/ArduinoJson/Variant/VariantData.hpp @@ -5,6 +5,7 @@ #pragma once #include "../Misc/SerializedValue.hpp" +#include "../Polyfills/gsl/not_null.hpp" #include "VariantContent.hpp" namespace ARDUINOJSON_NAMESPACE { @@ -179,9 +180,13 @@ class VariantData { } void setLinkedRaw(SerializedValue value) { - setType(VALUE_IS_LINKED_RAW); - _content.asRaw.data = value.data(); - _content.asRaw.size = value.size(); + if (value.data()) { + setType(VALUE_IS_LINKED_RAW); + _content.asRaw.data = value.data(); + _content.asRaw.size = value.size(); + } else { + setType(VALUE_IS_NULL); + } } template @@ -220,25 +225,26 @@ class VariantData { } void setLinkedString(const char *value) { - setType(VALUE_IS_LINKED_STRING); - _content.asString = value; + if (value) { + setType(VALUE_IS_LINKED_STRING); + _content.asString = value; + } else { + setType(VALUE_IS_NULL); + } } void setNull() { setType(VALUE_IS_NULL); } - void setOwnedString(const char *s) { + void setOwnedString(not_null s) { setType(VALUE_IS_OWNED_STRING); - _content.asString = s; + _content.asString = s.get(); } - template - bool setOwnedString(T value, MemoryPool *pool) { - char *dup = value.save(pool); - if (dup) { - setType(VALUE_IS_OWNED_STRING); - _content.asString = dup; + bool setOwnedString(const char *s) { + if (s) { + setOwnedString(make_not_null(s)); return true; } else { setType(VALUE_IS_NULL); @@ -246,6 +252,11 @@ class VariantData { } } + template + bool setOwnedString(T value, MemoryPool *pool) { + return setOwnedString(value.save(pool)); + } + void setUnsignedInteger(UInt value) { setType(VALUE_IS_POSITIVE_INTEGER); _content.asInteger = static_cast(value); diff --git a/src/ArduinoJson/Variant/VariantSlot.hpp b/src/ArduinoJson/Variant/VariantSlot.hpp index dda5eae0..f99ef2de 100644 --- a/src/ArduinoJson/Variant/VariantSlot.hpp +++ b/src/ArduinoJson/Variant/VariantSlot.hpp @@ -4,6 +4,7 @@ #pragma once +#include "../Polyfills/gsl/not_null.hpp" #include "../Polyfills/type_traits.hpp" #include "../Variant/VariantContent.hpp" @@ -67,14 +68,14 @@ class VariantSlot { _next = VariantSlotDiff(slot - this); } - void setOwnedKey(const char* k) { + void setOwnedKey(not_null k) { _flags |= KEY_IS_OWNED; - _key = k; + _key = k.get(); } - void setLinkedKey(const char* k) { + void setLinkedKey(not_null k) { _flags &= VALUE_MASK; - _key = k; + _key = k.get(); } const char* key() const { diff --git a/test/JsonDeserializer/deserializeJsonString.cpp b/test/JsonDeserializer/deserializeJsonString.cpp index 0501eaa6..cdfcf4c6 100644 --- a/test/JsonDeserializer/deserializeJsonString.cpp +++ b/test/JsonDeserializer/deserializeJsonString.cpp @@ -64,3 +64,11 @@ TEST_CASE("Invalid JSON string") { REQUIRE(deserializeJson(doc, input) == DeserializationError::InvalidInput); } } + +TEST_CASE("Not enough room to duplicate the string") { + DynamicJsonDocument doc(4); + + REQUIRE(deserializeJson(doc, "\"hello world!\"") == + DeserializationError::NoMemory); + REQUIRE(doc.isNull() == true); +} diff --git a/test/JsonVariant/isnull.cpp b/test/JsonVariant/isnull.cpp index aa5013c1..c3d86507 100644 --- a/test/JsonVariant/isnull.cpp +++ b/test/JsonVariant/isnull.cpp @@ -35,15 +35,40 @@ TEST_CASE("JsonVariant::isNull()") { REQUIRE(variant.isNull() == false); } - /* SECTION("return true when InvalidArray") { - variant.set(JsonArray()); - REQUIRE(variant.isNull() == true); - } - */ - /* SECTION("return true when InvalidObject") { - variant.set(JsonObject()); - REQUIRE(variant.isNull() == true); - }*/ + SECTION("return true after set(JsonArray())") { + variant.set(JsonArray()); + REQUIRE(variant.isNull() == true); + } + + SECTION("return true after set(JsonObject())") { + variant.set(JsonObject()); + REQUIRE(variant.isNull() == true); + } + + SECTION("return false after set('hello')") { + variant.set("hello"); + REQUIRE(variant.isNull() == false); + } + + SECTION("return true after set((char*)0)") { + variant.set(static_cast(0)); + REQUIRE(variant.isNull() == true); + } + + SECTION("return true after set((const char*)0)") { + variant.set(static_cast(0)); + REQUIRE(variant.isNull() == true); + } + + SECTION("return true after set(serialized((char*)0))") { + variant.set(serialized(static_cast(0))); + REQUIRE(variant.isNull() == true); + } + + SECTION("return true after set(serialized((const char*)0))") { + variant.set(serialized(static_cast(0))); + REQUIRE(variant.isNull() == true); + } SECTION("works with JsonVariantConst") { variant.set(42);