From 5a4d993f7d0429789f960988d973a3b5bc79db59 Mon Sep 17 00:00:00 2001 From: Benoit Blanchon Date: Tue, 1 Sep 2015 22:21:50 +0200 Subject: [PATCH] Fixed memory alignment, which made ESP8266 crash (issue #104) --- CHANGELOG.md | 5 +- .../ArduinoJson/Internals/BlockJsonBuffer.hpp | 2 +- include/ArduinoJson/JsonBuffer.hpp | 18 +++++- include/ArduinoJson/StaticJsonBuffer.hpp | 3 +- test/DynamicJsonBuffer_Basic_Tests.cpp | 13 ++++- test/Issue104.cpp | 56 ------------------- test/StaticJsonBuffer_Basic_Tests.cpp | 29 ++++++---- 7 files changed, 51 insertions(+), 75 deletions(-) delete mode 100644 test/Issue104.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index a7fbc997..7086cf98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,10 @@ ArduinoJson: change log v5.0.2 ------ -* Fixed Clang warning "register specifier is deprecated" (issue #102) * Fixed segmentation fault in `parseObject(String)` and `parseArray(String)`, when the - `StaticJsonBuffer` is too small to hold a copy of the string (issue #104) + `StaticJsonBuffer` is too small to hold a copy of the string +* Fixed Clang warning "register specifier is deprecated" (issue #102) +* Fixed memory alignment, which made ESP8266 crash (issue #104) * Fixed compilation on Visual Studio 2010 and 2012 (issue #107) v5.0.1 diff --git a/include/ArduinoJson/Internals/BlockJsonBuffer.hpp b/include/ArduinoJson/Internals/BlockJsonBuffer.hpp index a156f659..b92d7bcd 100644 --- a/include/ArduinoJson/Internals/BlockJsonBuffer.hpp +++ b/include/ArduinoJson/Internals/BlockJsonBuffer.hpp @@ -63,7 +63,7 @@ class BlockJsonBuffer : public JsonBuffer { void* allocInHead(size_t bytes) { void* p = _head->data + _head->size; - _head->size += bytes; + _head->size += round_size_up(bytes); return p; } diff --git a/include/ArduinoJson/JsonBuffer.hpp b/include/ArduinoJson/JsonBuffer.hpp index c0614534..e9bd9d05 100644 --- a/include/ArduinoJson/JsonBuffer.hpp +++ b/include/ArduinoJson/JsonBuffer.hpp @@ -103,19 +103,33 @@ class JsonBuffer { // Return a pointer to the allocated memory or NULL if allocation fails. virtual void *alloc(size_t size) = 0; + protected: + // Preserve aligment if nessary + static FORCE_INLINE size_t round_size_up(size_t bytes) { +#if defined ARDUINO_ARCH_AVR + // alignment isn't needed for 8-bit AVR + return bytes; +#else + const size_t x = sizeof(void *) - 1; + return (bytes + x) & ~x; +#endif + } + private: char *strdup(const char *, size_t); // Default value of nesting limit of parseArray() and parseObject(). // - // The nesting limit is a contain on the level of nesting allowed in the JSON + // The nesting limit is a contain on the level of nesting allowed in the + // JSON // string. // If set to 0, only a flat array or objects can be parsed. // If set to 1, the object can contain nested arrays or objects but only 1 // level deep. // And bigger values will allow more level of nesting. // - // The purpose of this feature is to prevent stack overflow that could lead to + // The purpose of this feature is to prevent stack overflow that could + // lead to // a security risk. static const uint8_t DEFAULT_LIMIT = 10; }; diff --git a/include/ArduinoJson/StaticJsonBuffer.hpp b/include/ArduinoJson/StaticJsonBuffer.hpp index e91f3307..9631bdba 100644 --- a/include/ArduinoJson/StaticJsonBuffer.hpp +++ b/include/ArduinoJson/StaticJsonBuffer.hpp @@ -21,11 +21,10 @@ class StaticJsonBuffer : public JsonBuffer { size_t capacity() const { return CAPACITY; } size_t size() const { return _size; } - protected: virtual void* alloc(size_t bytes) { if (_size + bytes > CAPACITY) return NULL; void* p = &_buffer[_size]; - _size += bytes; + _size += round_size_up(bytes); return p; } diff --git a/test/DynamicJsonBuffer_Basic_Tests.cpp b/test/DynamicJsonBuffer_Basic_Tests.cpp index bd8fa53b..30842659 100644 --- a/test/DynamicJsonBuffer_Basic_Tests.cpp +++ b/test/DynamicJsonBuffer_Basic_Tests.cpp @@ -22,9 +22,9 @@ TEST_F(DynamicJsonBuffer_Basic_Tests, InitialSizeIsZero) { TEST_F(DynamicJsonBuffer_Basic_Tests, SizeIncreasesAfterAlloc) { buffer.alloc(1); - ASSERT_EQ(1, buffer.size()); + ASSERT_LE(1, buffer.size()); buffer.alloc(1); - ASSERT_EQ(2, buffer.size()); + ASSERT_LE(2, buffer.size()); } TEST_F(DynamicJsonBuffer_Basic_Tests, ReturnDifferentPointer) { @@ -32,3 +32,12 @@ TEST_F(DynamicJsonBuffer_Basic_Tests, ReturnDifferentPointer) { void* p2 = buffer.alloc(2); ASSERT_NE(p1, p2); } + +TEST_F(DynamicJsonBuffer_Basic_Tests, Alignment) { + size_t mask = sizeof(void*) - 1; + + for (size_t size = 1; size <= sizeof(void*); size++) { + size_t addr = reinterpret_cast(buffer.alloc(1)); + ASSERT_EQ(0, addr & mask); + } +} diff --git a/test/Issue104.cpp b/test/Issue104.cpp deleted file mode 100644 index 154727f8..00000000 --- a/test/Issue104.cpp +++ /dev/null @@ -1,56 +0,0 @@ -// Copyright Benoit Blanchon 2014-2015 -// MIT License -// -// Arduino JSON library -// https://github.com/bblanchon/ArduinoJson - -#include -#define ARDUINOJSON_ENABLE_STD_STREAM -#include - -#define SOURCE "{\"mqtt.host\":\"mqtt.test.com\",\"mqtt.port\":1883}" - -#define SIZE_WITH_COPY (sizeof(SOURCE) + JSON_OBJECT_SIZE(2)) -#define SIZE_WITHOUT_COPY (JSON_OBJECT_SIZE(2)) - -template -void mustSucceedWith(S source) { - StaticJsonBuffer jsonBuffer; - ASSERT_TRUE(jsonBuffer.parseObject(source).success()); -} - -template -void mustFailWith(S source) { - StaticJsonBuffer jsonBuffer; - ASSERT_FALSE(jsonBuffer.parseObject(source).success()); -} - -TEST(Issue104, CharPtrSucceeds) { - char source[] = SOURCE; - mustSucceedWith(source); -} - -TEST(Issue104, CharPtrFails) { - char source[] = SOURCE; - mustFailWith(source); -} - -TEST(Issue104, ConstCharPtrSucceeds) { - mustSucceedWith(SOURCE); -} - -TEST(Issue104, ConstCharPtrFails) { - mustFailWith(SOURCE); -} - -TEST(Issue104, StringSucceeds) { - mustSucceedWith(SOURCE); -} - -TEST(Issue104, StringFails) { - mustFailWith(SOURCE); -} - -TEST(Issue104, TooSmallForStrDup) { - mustFailWith(SOURCE); -} diff --git a/test/StaticJsonBuffer_Basic_Tests.cpp b/test/StaticJsonBuffer_Basic_Tests.cpp index da1bb19a..30acd16e 100644 --- a/test/StaticJsonBuffer_Basic_Tests.cpp +++ b/test/StaticJsonBuffer_Basic_Tests.cpp @@ -13,11 +13,11 @@ using namespace ArduinoJson; class StaticJsonBuffer_Basic_Tests : public testing::Test { protected: - StaticJsonBuffer<42> buffer; + StaticJsonBuffer<64> buffer; }; TEST_F(StaticJsonBuffer_Basic_Tests, CapacityMatchTemplateParameter) { - ASSERT_EQ(42, buffer.capacity()); + ASSERT_EQ(64, buffer.capacity()); } TEST_F(StaticJsonBuffer_Basic_Tests, InitialSizeIsZero) { @@ -26,34 +26,43 @@ TEST_F(StaticJsonBuffer_Basic_Tests, InitialSizeIsZero) { TEST_F(StaticJsonBuffer_Basic_Tests, GrowsAfterAlloc) { buffer.alloc(1); - ASSERT_EQ(1, buffer.size()); + ASSERT_LE(1, buffer.size()); buffer.alloc(1); - ASSERT_EQ(2, buffer.size()); + ASSERT_LE(2, buffer.size()); } TEST_F(StaticJsonBuffer_Basic_Tests, DoesntGrowWhenFull) { - buffer.alloc(42); + buffer.alloc(64); buffer.alloc(1); - ASSERT_EQ(42, buffer.size()); + ASSERT_EQ(64, buffer.size()); } TEST_F(StaticJsonBuffer_Basic_Tests, DoesntGrowWhenTooSmall) { - buffer.alloc(43); + buffer.alloc(65); ASSERT_EQ(0, buffer.size()); } TEST_F(StaticJsonBuffer_Basic_Tests, ReturnsNonNull) { - void *p = buffer.alloc(42); + void *p = buffer.alloc(64); ASSERT_NE(static_cast(0), p); } TEST_F(StaticJsonBuffer_Basic_Tests, ReturnsNullWhenFull) { - buffer.alloc(42); + buffer.alloc(64); void *p = buffer.alloc(1); ASSERT_EQ(NULL, p); } TEST_F(StaticJsonBuffer_Basic_Tests, ReturnsNullWhenTooSmall) { - void *p = buffer.alloc(43); + void *p = buffer.alloc(65); ASSERT_EQ(NULL, p); } + +TEST_F(StaticJsonBuffer_Basic_Tests, Alignment) { + size_t mask = sizeof(void *) - 1; + + for (size_t size = 1; size <= sizeof(void *); size++) { + size_t addr = reinterpret_cast(buffer.alloc(1)); + ASSERT_EQ(0, addr & mask); + } +}