Made deserializeJson() more picky about trailing characters (closes #980)

CHANGELOG.md

@@ -6,6 +6,7 @@ HEAD
 
 * Fixed `deserializeJson()` silently accepting a `Stream*` (issue #978)
 * Fixed invalid result from `operator|` (issue #981)
+* Made `deserializeJson()` more picky about trailing characters (issue #980)
 
 > ### BREAKING CHANGE
 > 

src/ArduinoJson/Deserialization/ArduinoStreamReader.hpp

@@ -12,22 +12,14 @@ namespace ARDUINOJSON_NAMESPACE {
 
 struct ArduinoStreamReader {
   Stream& _stream;
-  char _current;
-  bool _ended;
 
  public:
-  explicit ArduinoStreamReader(Stream& stream)
-      : _stream(stream), _current(0), _ended(false) {}
+  explicit ArduinoStreamReader(Stream& stream) : _stream(stream) {}
 
-  char read() {
+  int read() {
     // don't use _stream.read() as it ignores the timeout
-    char c = 0;
-    _ended = _stream.readBytes(&c, 1) == 0;
-    return c;
-  }
-
-  bool ended() const {
-    return _ended;
+    uint8_t c;
+    return _stream.readBytes(&c, 1) ? c : -1;
   }
 };
 

src/ArduinoJson/Deserialization/CharPointerReader.hpp

@@ -23,13 +23,8 @@ class UnsafeCharPointerReader {
   explicit UnsafeCharPointerReader(const char* ptr)
       : _ptr(ptr ? ptr : reinterpret_cast<const char*>("")) {}
 
-  char read() {
-    return static_cast<char>(*_ptr++);
-  }
-
-  bool ended() const {
-    // we cannot know, that's why it's unsafe
-    return false;
+  int read() {
+    return static_cast<unsigned char>(*_ptr++);
   }
 };
 
@@ -41,12 +36,11 @@ class SafeCharPointerReader {
   explicit SafeCharPointerReader(const char* ptr, size_t len)
       : _ptr(ptr ? ptr : reinterpret_cast<const char*>("")), _end(_ptr + len) {}
 
-  char read() {
-    return static_cast<char>(*_ptr++);
-  }
-
-  bool ended() const {
-    return _ptr == _end;
+  int read() {
+    if (_ptr < _end)
+      return static_cast<unsigned char>(*_ptr++);
+    else
+      return -1;
   }
 };
 

src/ArduinoJson/Deserialization/FlashStringReader.hpp

@@ -14,14 +14,9 @@ class UnsafeFlashStringReader {
   explicit UnsafeFlashStringReader(const __FlashStringHelper* ptr)
       : _ptr(reinterpret_cast<const char*>(ptr)) {}
 
-  char read() {
+  int read() {
     return pgm_read_byte_near(_ptr++);
   }
-
-  bool ended() const {
-    // this reader cannot detect the end
-    return false;
-  }
 };
 
 class SafeFlashStringReader {
@@ -32,12 +27,11 @@ class SafeFlashStringReader {
   explicit SafeFlashStringReader(const __FlashStringHelper* ptr, size_t size)
       : _ptr(reinterpret_cast<const char*>(ptr)), _end(_ptr + size) {}
 
-  char read() {
-    return pgm_read_byte_near(_ptr++);
-  }
-
-  bool ended() const {
-    return _ptr == _end;
+  int read() {
+    if (_ptr < _end)
+      return pgm_read_byte_near(_ptr++);
+    else
+      return -1;
   }
 };
 

src/ArduinoJson/Deserialization/IteratorReader.hpp

@@ -14,12 +14,11 @@ class IteratorReader {
   explicit IteratorReader(TIterator begin, TIterator end)
       : _ptr(begin), _end(end) {}
 
-  bool ended() const {
-    return _ptr == _end;
-  }
-
-  char read() {
-    return char(*_ptr++);
+  int read() {
+    if (_ptr < _end)
+      return static_cast<unsigned char>(*_ptr++);
+    else
+      return -1;
   }
 };
 

src/ArduinoJson/Deserialization/StdStreamReader.hpp

@@ -18,12 +18,8 @@ class StdStreamReader {
   explicit StdStreamReader(std::istream& stream)
       : _stream(stream), _current(0) {}
 
-  bool ended() const {
-    return _stream.eof();
-  }
-
-  char read() {
-    return static_cast<char>(_stream.get());
+  int read() {
+    return _stream.get();
   }
 
  private:

src/ArduinoJson/Json/JsonDeserializer.hpp

@@ -28,19 +28,14 @@ class JsonDeserializer {
         _nestingLimit(nestingLimit),
         _loaded(false) {}
   DeserializationError parse(VariantData &variant) {
-    DeserializationError err = skipSpacesAndComments();
-    if (err) return err;
+    DeserializationError err = parseVariant(variant);
 
-    switch (current()) {
-      case '[':
-        return parseArray(variant.toArray());
-
-      case '{':
-        return parseObject(variant.toObject());
-
-      default:
-        return parseValue(variant);
+    if (!err && _current != 0 && !variant.isEnclosed()) {
+      // We don't detect trailing characters earlier, so we need to check now
+      err = DeserializationError::InvalidInput;
     }
+
+    return err;
   }
 
  private:
@@ -48,10 +43,8 @@ class JsonDeserializer {
 
   char current() {
     if (!_loaded) {
-      if (_reader.ended())
-        _current = 0;
-      else
-        _current = _reader.read();
+      int c = _reader.read();
+      _current = static_cast<char>(c > 0 ? c : 0);
       _loaded = true;
     }
     return _current;
@@ -67,6 +60,26 @@ class JsonDeserializer {
     return true;
   }
 
+  DeserializationError parseVariant(VariantData &variant) {
+    DeserializationError err = skipSpacesAndComments();
+    if (err) return err;
+
+    switch (current()) {
+      case '[':
+        return parseArray(variant.toArray());
+
+      case '{':
+        return parseObject(variant.toObject());
+
+      case '\"':
+      case '\'':
+        return parseStringValue(variant);
+
+      default:
+        return parseNumericValue(variant);
+    }
+  }
+
   DeserializationError parseArray(CollectionData &array) {
     if (_nestingLimit == 0) return DeserializationError::TooDeep;
 
@@ -88,7 +101,7 @@ class JsonDeserializer {
 
       // 1 - Parse value
       _nestingLimit--;
-      err = parse(*value);
+      err = parseVariant(*value);
       _nestingLimit++;
       if (err) return err;
 
@@ -134,7 +147,7 @@ class JsonDeserializer {
 
       // Parse value
       _nestingLimit--;
-      err = parse(*slot->data());
+      err = parseVariant(*slot->data());
       _nestingLimit++;
       if (err) return err;
 
@@ -152,14 +165,6 @@ class JsonDeserializer {
     }
   }
 
-  DeserializationError parseValue(VariantData &variant) {
-    if (isQuote(current())) {
-      return parseStringValue(variant);
-    } else {
-      return parseNumericValue(variant);
-    }
-  }
-
   DeserializationError parseKey(const char *&key) {
     if (isQuote(current())) {
       return parseQuotedString(key);

src/ArduinoJson/MsgPack/MsgPackDeserializer.hpp

@@ -134,17 +134,10 @@ class MsgPackDeserializer {
   // Prevent VS warning "assignment operator could not be generated"
   MsgPackDeserializer &operator=(const MsgPackDeserializer &);
 
-  bool skip(uint8_t n) {
-    while (n--) {
-      if (_reader.ended()) return false;
-      _reader.read();
-    }
-    return true;
-  }
-
   bool readByte(uint8_t &value) {
-    if (_reader.ended()) return false;
-    value = static_cast<uint8_t>(_reader.read());
+    int c = _reader.read();
+    if (c < 0) return false;
+    value = static_cast<uint8_t>(c);
     return true;
   }
 

src/ArduinoJson/Variant/VariantData.hpp

@@ -177,6 +177,10 @@ class VariantData {
     return type() == VALUE_IS_NULL;
   }
 
+  bool isEnclosed() const {
+    return isCollection() || isString();
+  }
+
   void remove(size_t index) {
     if (isArray()) _content.asCollection.remove(index);
   }

test/JsonDeserializer/invalid_input.cpp

@@ -8,7 +8,8 @@
 
 TEST_CASE("Invalid JSON input") {
   const char* testCases[] = {"'\\u'",     "'\\u000g'", "'\\u000'", "'\\u000G'",
-                             "'\\u000/'", "\\x1234",   "6a9"};
+                             "'\\u000/'", "\\x1234",   "6a9",      "1,",
+                             "2]",        "3}"};
   const size_t testCount = sizeof(testCases) / sizeof(testCases[0]);
 
   DynamicJsonDocument doc(4096);

test/Misc/CMakeLists.txt

@@ -5,6 +5,7 @@
 add_executable(MiscTests
 	conflicts.cpp
 	FloatParts.cpp
+	StreamReader.cpp
 	StringWriter.cpp
 	TypeTraits.cpp
 	unsigned_char.cpp

test/Misc/StreamReader.cpp

@@ -0,0 +1,33 @@
+// ArduinoJson - arduinojson.org
+// Copyright Benoit Blanchon 2014-2019
+// MIT License
+
+#include <ArduinoJson.h>
+#include <catch.hpp>
+
+using namespace ARDUINOJSON_NAMESPACE;
+
+TEST_CASE("StdStreamReader") {
+  std::istringstream src("\x01\xFF");
+  StdStreamReader reader(src);
+
+  REQUIRE(reader.read() == 0x01);
+  REQUIRE(reader.read() == 0xFF);
+  REQUIRE(reader.read() == -1);
+}
+
+TEST_CASE("SafeCharPointerReader") {
+  SafeCharPointerReader reader("\x01\xFF", 2);
+
+  REQUIRE(reader.read() == 0x01);
+  REQUIRE(reader.read() == 0xFF);
+  REQUIRE(reader.read() == -1);
+}
+
+TEST_CASE("UnsafeCharPointerReader") {
+  UnsafeCharPointerReader reader("\x01\xFF");
+
+  REQUIRE(reader.read() == 0x01);
+  REQUIRE(reader.read() == 0xFF);
+  REQUIRE(reader.read() == 0);
+}