From 8d37939086969c4cc97be56ec105ee4850e3cd9f Mon Sep 17 00:00:00 2001 From: Benoit Blanchon Date: Sat, 5 Sep 2020 10:54:46 +0200 Subject: [PATCH] Added `JsonDocument::overflowed()` (closes #1358) --- CHANGELOG.md | 1 + extras/tests/JsonDocument/CMakeLists.txt | 1 + extras/tests/JsonDocument/overflowed.cpp | 79 +++++++++++++++++++ extras/tests/MemoryPool/StringCopier.cpp | 36 ++++----- extras/tests/Misc/Utf8.cpp | 4 +- .../Deserialization/deserialize.hpp | 15 ++-- src/ArduinoJson/Document/JsonDocument.hpp | 5 ++ src/ArduinoJson/Json/JsonDeserializer.hpp | 8 +- src/ArduinoJson/Memory/MemoryPool.hpp | 21 ++++- .../MsgPack/MsgPackDeserializer.hpp | 4 +- .../StringStorage/StringCopier.hpp | 12 ++- src/ArduinoJson/StringStorage/StringMover.hpp | 4 +- .../StringStorage/StringStorage.hpp | 29 ++----- 13 files changed, 155 insertions(+), 64 deletions(-) create mode 100644 extras/tests/JsonDocument/overflowed.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 05f1b1bc..6d68eb9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ HEAD ---- * Added a build failure when nullptr is defined as a macro (issue #1355) +* Added `JsonDocument::overflowed()` which tells if the memory pool was too small (issue #1358) v6.16.1 (2020-08-04) ------- diff --git a/extras/tests/JsonDocument/CMakeLists.txt b/extras/tests/JsonDocument/CMakeLists.txt index 5a083a68..ce00ab2f 100644 --- a/extras/tests/JsonDocument/CMakeLists.txt +++ b/extras/tests/JsonDocument/CMakeLists.txt @@ -11,6 +11,7 @@ add_executable(JsonDocumentTests DynamicJsonDocument.cpp isNull.cpp nesting.cpp + overflowed.cpp remove.cpp shrinkToFit.cpp size.cpp diff --git a/extras/tests/JsonDocument/overflowed.cpp b/extras/tests/JsonDocument/overflowed.cpp new file mode 100644 index 00000000..74aa5716 --- /dev/null +++ b/extras/tests/JsonDocument/overflowed.cpp @@ -0,0 +1,79 @@ +// ArduinoJson - arduinojson.org +// Copyright Benoit Blanchon 2014-2020 +// MIT License + +#include +#include + +TEST_CASE("JsonDocument::overflowed()") { + SECTION("returns false on a fresh object") { + StaticJsonDocument<0> doc; + CHECK(doc.overflowed() == false); + } + + SECTION("returns true after a failed insertion") { + StaticJsonDocument<0> doc; + doc.add(0); + CHECK(doc.overflowed() == true); + } + + SECTION("returns false after successful insertion") { + StaticJsonDocument doc; + doc.add(0); + CHECK(doc.overflowed() == false); + } + + SECTION("returns true after a failed string copy") { + StaticJsonDocument doc; + doc.add(std::string("example")); + CHECK(doc.overflowed() == true); + } + + SECTION("returns false after a successful string copy") { + StaticJsonDocument doc; + doc.add(std::string("example")); + CHECK(doc.overflowed() == false); + } + + SECTION("returns true after a failed deserialization") { + StaticJsonDocument doc; + deserializeJson(doc, "[\"example\"]"); + CHECK(doc.overflowed() == true); + } + + SECTION("returns false after a successful deserialization") { + StaticJsonDocument doc; + deserializeJson(doc, "[\"example\"]"); + CHECK(doc.overflowed() == false); + } + + SECTION("returns false after clear()") { + StaticJsonDocument<0> doc; + doc.add(0); + doc.clear(); + CHECK(doc.overflowed() == false); + } + + SECTION("remains false after shrinkToFit()") { + DynamicJsonDocument doc(JSON_ARRAY_SIZE(1)); + doc.add(0); + doc.shrinkToFit(); + CHECK(doc.overflowed() == false); + } + + SECTION("remains true after shrinkToFit()") { + DynamicJsonDocument doc(JSON_ARRAY_SIZE(1)); + doc.add(0); + doc.add(0); + doc.shrinkToFit(); + CHECK(doc.overflowed() == true); + } + + SECTION("return false after garbageCollect()") { + DynamicJsonDocument doc(JSON_ARRAY_SIZE(1)); + doc.add(0); + doc.add(0); + doc.garbageCollect(); + CHECK(doc.overflowed() == false); + } +} diff --git a/extras/tests/MemoryPool/StringCopier.cpp b/extras/tests/MemoryPool/StringCopier.cpp index 236c81ae..a4d38713 100644 --- a/extras/tests/MemoryPool/StringCopier.cpp +++ b/extras/tests/MemoryPool/StringCopier.cpp @@ -12,9 +12,9 @@ TEST_CASE("StringCopier") { SECTION("Works when buffer is big enough") { MemoryPool pool(buffer, addPadding(JSON_STRING_SIZE(6))); - StringCopier str; + StringCopier str(pool); - str.startString(&pool); + str.startString(); str.append("hello"); str.append('\0'); @@ -24,9 +24,9 @@ TEST_CASE("StringCopier") { SECTION("Returns null when too small") { MemoryPool pool(buffer, sizeof(void*)); - StringCopier str; + StringCopier str(pool); - str.startString(&pool); + str.startString(); str.append("hello world!"); REQUIRE(str.isValid() == false); @@ -34,22 +34,22 @@ TEST_CASE("StringCopier") { SECTION("Increases size of memory pool") { MemoryPool pool(buffer, addPadding(JSON_STRING_SIZE(6))); - StringCopier str; + StringCopier str(pool); - str.startString(&pool); + str.startString(); str.append('h'); - str.save(&pool); + str.save(); REQUIRE(1 == pool.size()); } } -static const char* addStringToPool(MemoryPool* pool, const char* s) { - StringCopier str; - str.startString(pool); +static const char* addStringToPool(MemoryPool& pool, const char* s) { + StringCopier str(pool); + str.startString(); str.append(s); str.append('\0'); - return str.save(pool); + return str.save(); } TEST_CASE("StringCopier::save() deduplicates strings") { @@ -57,9 +57,9 @@ TEST_CASE("StringCopier::save() deduplicates strings") { MemoryPool pool(buffer, 4096); SECTION("Basic") { - const char* s1 = addStringToPool(&pool, "hello"); - const char* s2 = addStringToPool(&pool, "world"); - const char* s3 = addStringToPool(&pool, "hello"); + const char* s1 = addStringToPool(pool, "hello"); + const char* s2 = addStringToPool(pool, "world"); + const char* s3 = addStringToPool(pool, "hello"); REQUIRE(s1 == s3); REQUIRE(s2 != s3); @@ -67,16 +67,16 @@ TEST_CASE("StringCopier::save() deduplicates strings") { } SECTION("Requires terminator") { - const char* s1 = addStringToPool(&pool, "hello world"); - const char* s2 = addStringToPool(&pool, "hello"); + const char* s1 = addStringToPool(pool, "hello world"); + const char* s2 = addStringToPool(pool, "hello"); REQUIRE(s2 != s1); REQUIRE(pool.size() == 12 + 6); } SECTION("Don't overrun") { - const char* s1 = addStringToPool(&pool, "hello world"); - const char* s2 = addStringToPool(&pool, "wor"); + const char* s1 = addStringToPool(pool, "hello world"); + const char* s2 = addStringToPool(pool, "wor"); REQUIRE(s2 != s1); REQUIRE(pool.size() == 12 + 4); diff --git a/extras/tests/Misc/Utf8.cpp b/extras/tests/Misc/Utf8.cpp index e50def69..cb0464c1 100644 --- a/extras/tests/Misc/Utf8.cpp +++ b/extras/tests/Misc/Utf8.cpp @@ -12,8 +12,8 @@ using namespace ARDUINOJSON_NAMESPACE; static void testCodepoint(uint32_t codepoint, std::string expected) { char buffer[4096]; MemoryPool pool(buffer, 4096); - StringCopier str; - str.startString(&pool); + StringCopier str(pool); + str.startString(); CAPTURE(codepoint); Utf8::encodeCodepoint(codepoint, str); diff --git a/src/ArduinoJson/Deserialization/deserialize.hpp b/src/ArduinoJson/Deserialization/deserialize.hpp index 0c9e3153..7b15e2da 100644 --- a/src/ArduinoJson/Deserialization/deserialize.hpp +++ b/src/ArduinoJson/Deserialization/deserialize.hpp @@ -32,8 +32,9 @@ deserialize(JsonDocument &doc, const TString &input, NestingLimit nestingLimit, TFilter filter) { Reader reader(input); doc.clear(); - return makeDeserializer(doc.memoryPool(), reader, - makeStringStorage(input)) + return makeDeserializer( + doc.memoryPool(), reader, + makeStringStorage(input, doc.memoryPool())) .parse(doc.data(), filter, nestingLimit); } // @@ -47,8 +48,9 @@ DeserializationError deserialize(JsonDocument &doc, TChar *input, TFilter filter) { BoundedReader reader(input, inputSize); doc.clear(); - return makeDeserializer(doc.memoryPool(), reader, - makeStringStorage(input)) + return makeDeserializer( + doc.memoryPool(), reader, + makeStringStorage(input, doc.memoryPool())) .parse(doc.data(), filter, nestingLimit); } // @@ -60,8 +62,9 @@ DeserializationError deserialize(JsonDocument &doc, TStream &input, NestingLimit nestingLimit, TFilter filter) { Reader reader(input); doc.clear(); - return makeDeserializer(doc.memoryPool(), reader, - makeStringStorage(input)) + return makeDeserializer( + doc.memoryPool(), reader, + makeStringStorage(input, doc.memoryPool())) .parse(doc.data(), filter, nestingLimit); } diff --git a/src/ArduinoJson/Document/JsonDocument.hpp b/src/ArduinoJson/Document/JsonDocument.hpp index 7c3a844e..13cb4911 100644 --- a/src/ArduinoJson/Document/JsonDocument.hpp +++ b/src/ArduinoJson/Document/JsonDocument.hpp @@ -48,6 +48,10 @@ class JsonDocument : public Visitable { return _pool.size(); } + bool overflowed() const { + return _pool.overflowed(); + } + size_t nesting() const { return _data.nesting(); } @@ -81,6 +85,7 @@ class JsonDocument : public Visitable { return _pool; } + // for internal use only VariantData& data() { return _data; } diff --git a/src/ArduinoJson/Json/JsonDeserializer.hpp b/src/ArduinoJson/Json/JsonDeserializer.hpp index 0f825509..9acf3b53 100644 --- a/src/ArduinoJson/Json/JsonDeserializer.hpp +++ b/src/ArduinoJson/Json/JsonDeserializer.hpp @@ -241,7 +241,7 @@ class JsonDeserializer { if (!variant) { // Save key in memory pool. // This MUST be done before adding the slot. - key = _stringStorage.save(_pool); + key = _stringStorage.save(); // Allocate slot in object VariantSlot *slot = object.addSlot(_pool); @@ -334,7 +334,7 @@ class JsonDeserializer { } bool parseKey() { - _stringStorage.startString(_pool); + _stringStorage.startString(); if (isQuote(current())) { return parseQuotedString(); } else { @@ -343,10 +343,10 @@ class JsonDeserializer { } bool parseStringValue(VariantData &variant) { - _stringStorage.startString(_pool); + _stringStorage.startString(); if (!parseQuotedString()) return false; - const char *value = _stringStorage.save(_pool); + const char *value = _stringStorage.save(); variant.setString(make_not_null(value), typename TStringStorage::storage_policy()); return true; diff --git a/src/ArduinoJson/Memory/MemoryPool.hpp b/src/ArduinoJson/Memory/MemoryPool.hpp index cb17334a..135d8ee5 100644 --- a/src/ArduinoJson/Memory/MemoryPool.hpp +++ b/src/ArduinoJson/Memory/MemoryPool.hpp @@ -29,7 +29,8 @@ class MemoryPool { : _begin(buf), _left(buf), _right(buf ? buf + capa : 0), - _end(buf ? buf + capa : 0) { + _end(buf ? buf + capa : 0), + _overflowed(false) { ARDUINOJSON_ASSERT(isAligned(_begin)); ARDUINOJSON_ASSERT(isAligned(_right)); ARDUINOJSON_ASSERT(isAligned(_end)); @@ -48,6 +49,10 @@ class MemoryPool { return size_t(_left - _begin + _end - _right); } + bool overflowed() const { + return _overflowed; + } + VariantSlot* allocVariant() { return allocRight(); } @@ -91,9 +96,14 @@ class MemoryPool { return str; } + void markAsOverflowed() { + _overflowed = true; + } + void clear() { _left = _begin; _right = _end; + _overflowed = false; } bool canAlloc(size_t bytes) const { @@ -171,8 +181,10 @@ class MemoryPool { #endif char* allocString(size_t n) { - if (!canAlloc(n)) + if (!canAlloc(n)) { + _overflowed = true; return 0; + } char* s = _left; _left += n; checkInvariants(); @@ -185,13 +197,16 @@ class MemoryPool { } void* allocRight(size_t bytes) { - if (!canAlloc(bytes)) + if (!canAlloc(bytes)) { + _overflowed = true; return 0; + } _right -= bytes; return _right; } char *_begin, *_left, *_right, *_end; + bool _overflowed; }; } // namespace ARDUINOJSON_NAMESPACE diff --git a/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp b/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp index 701f7ed2..d068c871 100644 --- a/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp +++ b/src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp @@ -239,7 +239,7 @@ class MsgPackDeserializer { } bool readString(const char *&result, size_t n) { - _stringStorage.startString(_pool); + _stringStorage.startString(); for (; n; --n) { uint8_t c; if (!readBytes(c)) @@ -252,7 +252,7 @@ class MsgPackDeserializer { return false; } - result = _stringStorage.save(_pool); + result = _stringStorage.save(); return true; } diff --git a/src/ArduinoJson/StringStorage/StringCopier.hpp b/src/ArduinoJson/StringStorage/StringCopier.hpp index e5558b7a..3c5f8e31 100644 --- a/src/ArduinoJson/StringStorage/StringCopier.hpp +++ b/src/ArduinoJson/StringStorage/StringCopier.hpp @@ -10,14 +10,16 @@ namespace ARDUINOJSON_NAMESPACE { class StringCopier { public: - void startString(MemoryPool* pool) { - pool->getFreeZone(&_ptr, &_capacity); + StringCopier(MemoryPool& pool) : _pool(&pool) {} + + void startString() { + _pool->getFreeZone(&_ptr, &_capacity); _size = 0; } - const char* save(MemoryPool* pool) { + const char* save() { ARDUINOJSON_ASSERT(_ptr); - return pool->saveStringFromFreeZone(_size); + return _pool->saveStringFromFreeZone(_size); } void append(const char* s) { @@ -34,6 +36,7 @@ class StringCopier { if (_size >= _capacity) { _ptr = 0; + _pool->markAsOverflowed(); return; } @@ -51,6 +54,7 @@ class StringCopier { typedef storage_policies::store_by_copy storage_policy; private: + MemoryPool* _pool; char* _ptr; size_t _size; size_t _capacity; diff --git a/src/ArduinoJson/StringStorage/StringMover.hpp b/src/ArduinoJson/StringStorage/StringMover.hpp index 724cbb87..e2b24fb5 100644 --- a/src/ArduinoJson/StringStorage/StringMover.hpp +++ b/src/ArduinoJson/StringStorage/StringMover.hpp @@ -13,11 +13,11 @@ class StringMover { public: StringMover(char* ptr) : _writePtr(ptr) {} - void startString(MemoryPool*) { + void startString() { _startPtr = _writePtr; } - const char* save(MemoryPool*) const { + const char* save() const { return _startPtr; } diff --git a/src/ArduinoJson/StringStorage/StringStorage.hpp b/src/ArduinoJson/StringStorage/StringStorage.hpp index 07c28d38..ad71d125 100644 --- a/src/ArduinoJson/StringStorage/StringStorage.hpp +++ b/src/ArduinoJson/StringStorage/StringStorage.hpp @@ -9,32 +9,15 @@ namespace ARDUINOJSON_NAMESPACE { -template -struct StringStorage { - typedef StringCopier type; - - static type create(TInput&) { - return type(); - } -}; - -template -struct StringStorage::value>::type> { - typedef StringMover type; - - static type create(TChar* input) { - return type(reinterpret_cast(input)); - } -}; - template -typename StringStorage::type makeStringStorage(TInput& input) { - return StringStorage::create(input); +StringCopier makeStringStorage(TInput&, MemoryPool& pool) { + return StringCopier(pool); } template -typename StringStorage::type makeStringStorage(TChar* input) { - return StringStorage::create(input); +StringMover makeStringStorage( + TChar* input, MemoryPool&, + typename enable_if::value>::type* = 0) { + return StringMover(reinterpret_cast(input)); } } // namespace ARDUINOJSON_NAMESPACE