Fixed null values that could be pass to strcmp() (closes #745)

.travis.yml

@@ -37,7 +37,7 @@ matrix:
         apt:
           sources: ['ubuntu-toolchain-r-test']
           packages: ['g++-5']
-      env: SCRIPT=cmake GCC=5 SANITIZE=undefined
+      env: SCRIPT=cmake GCC=5
     - compiler: gcc
       addons:
         apt:

CHANGELOG.md

@@ -6,6 +6,7 @@ HEAD
 
 * Fixed `JsonBuffer::parse()` not respecting nesting limit correctly (issue #693)
 * Fixed inconsistencies in nesting level counting (PR #695 from Zhenyu Wu)
+* Fixed null values that could be pass to `strcmp()` (PR #745 from Mike Karlesky)
 
 v5.13.1
 -------

src/ArduinoJson/JsonObject.hpp

@@ -286,15 +286,21 @@ class JsonObject : public Internals::JsonPrintable<JsonObject>,
 
   template <typename TStringRef, typename TValueRef>
   bool set_impl(TStringRef key, TValueRef value) {
+    // ignore null key
+    if (Internals::StringTraits<TStringRef>::is_null(key)) return false;
+
+    // search a matching key
     iterator it = findKey<TStringRef>(key);
     if (it == end()) {
+      // add the key
       it = Internals::List<JsonPair>::add();
       if (it == end()) return false;
-
       bool key_ok =
           Internals::ValueSaver<TStringRef>::save(_buffer, it->key, key);
       if (!key_ok) return false;
     }
+
+    // save the value
     return Internals::ValueSaver<TValueRef>::save(_buffer, it->value, value);
   }
 
@@ -318,5 +324,5 @@ struct JsonVariantDefault<JsonObject> {
     return JsonObject::invalid();
   }
 };
-}
-}
+}  // namespace Internals
+}  // namespace ArduinoJson

src/ArduinoJson/JsonVariantComparisons.hpp

@@ -129,10 +129,11 @@ class JsonVariantComparisons {
     if (is<JsonObject>() && right.template is<JsonObject>())
       return as<JsonObject>() == right.template as<JsonObject>();
     if (is<char *>() && right.template is<char *>())
-      return strcmp(as<char *>(), right.template as<char *>()) == 0;
+      return StringTraits<const char *>::equals(as<char *>(),
+                                                right.template as<char *>());
 
     return false;
   }
 };
-}
-}
+}  // namespace Internals
+}  // namespace ArduinoJson

src/ArduinoJson/StringTraits/CharPointer.hpp

@@ -30,7 +30,9 @@ struct CharPointerTraits {
   };
 
   static bool equals(const TChar* str, const char* expected) {
-    return strcmp(reinterpret_cast<const char*>(str), expected) == 0;
+    const char* actual = reinterpret_cast<const char*>(str);
+    if (!actual || !expected) return actual == expected;
+    return strcmp(actual, expected) == 0;
   }
 
   static bool is_null(const TChar* str) {
@@ -58,5 +60,5 @@ struct CharPointerTraits {
 template <typename TChar>
 struct StringTraits<TChar*, typename EnableIf<IsChar<TChar>::value>::type>
     : CharPointerTraits<TChar> {};
-}
-}
+}  // namespace Internals
+}  // namespace ArduinoJson

src/ArduinoJson/StringTraits/FlashString.hpp

@@ -31,7 +31,9 @@ struct StringTraits<const __FlashStringHelper*, void> {
   };
 
   static bool equals(const __FlashStringHelper* str, const char* expected) {
-    return strcmp_P(expected, (const char*)str) == 0;
+    const char* actual = reinterpret_cast<const char*>(str);
+    if (!actual || !expected) return actual == expected;
+    return strcmp_P(expected, actual) == 0;
   }
 
   static bool is_null(const __FlashStringHelper* str) {
@@ -53,7 +55,7 @@ struct StringTraits<const __FlashStringHelper*, void> {
   static const bool has_equals = true;
   static const bool should_duplicate = true;
 };
-}
-}
+}  // namespace Internals
+}  // namespace ArduinoJson
 
 #endif

src/ArduinoJson/StringTraits/StdString.hpp

@@ -40,7 +40,10 @@ struct StdStringTraits {
   };
 
   static bool equals(const TString& str, const char* expected) {
-    return 0 == strcmp(str.c_str(), expected);
+    // Arduino's String::c_str() can return NULL
+    const char* actual = str.c_str();
+    if (!actual || !expected) return actual == expected;
+    return 0 == strcmp(actual, expected);
   }
 
   static void append(TString& str, char c) {
@@ -68,7 +71,7 @@ struct StringTraits<StringSumHelper, void> : StdStringTraits<StringSumHelper> {
 template <>
 struct StringTraits<std::string, void> : StdStringTraits<std::string> {};
 #endif
-}
-}
+}  // namespace Internals
+}  // namespace ArduinoJson
 
 #endif

test/JsonObject/subscript.cpp

@@ -149,4 +149,15 @@ TEST_CASE("JsonObject::operator[]") {
     const size_t expectedSize = JSON_OBJECT_SIZE(1) + 12;
     REQUIRE(expectedSize <= _jsonBuffer.size());
   }
+
+  SECTION("should ignore null key") {
+    // object must have a value to make a call to strcmp()
+    _object["dummy"] = 42;
+
+    const char* null = 0;
+    _object[null] = 666;
+
+    REQUIRE(_object.size() == 1);
+    REQUIRE(_object[null] == 0);
+  }
 }

test/JsonVariant/compare.cpp

@@ -5,6 +5,8 @@
 #include <ArduinoJson.h>
 #include <catch.hpp>
 
+static const char* null = 0;
+
 template <typename T>
 void checkEquals(JsonVariant a, T b) {
   REQUIRE(b == a);
@@ -96,38 +98,69 @@ TEST_CASE("JsonVariant comparisons") {
     checkComparisons<unsigned short>(122, 123, 124);
   }
 
+  SECTION("null") {
+    JsonVariant variant = null;
+
+    REQUIRE(variant == variant);
+    REQUIRE_FALSE(variant != variant);
+
+    REQUIRE(variant == null);
+    REQUIRE_FALSE(variant != null);
+
+    REQUIRE(variant != "null");
+    REQUIRE_FALSE(variant == "null");
+  }
+
   SECTION("StringLiteral") {
     DynamicJsonBuffer jsonBuffer;
     JsonVariant variant = jsonBuffer.parse("\"hello\"");
 
+    REQUIRE(variant == variant);
+    REQUIRE_FALSE(variant != variant);
+
     REQUIRE(variant == "hello");
     REQUIRE_FALSE(variant != "hello");
 
     REQUIRE(variant != "world");
     REQUIRE_FALSE(variant == "world");
 
+    REQUIRE(variant != null);
+    REQUIRE_FALSE(variant == null);
+
     REQUIRE("hello" == variant);
     REQUIRE_FALSE("hello" != variant);
 
     REQUIRE("world" != variant);
     REQUIRE_FALSE("world" == variant);
+
+    REQUIRE(null != variant);
+    REQUIRE_FALSE(null == variant);
   }
 
   SECTION("String") {
     DynamicJsonBuffer jsonBuffer;
     JsonVariant variant = jsonBuffer.parse("\"hello\"");
 
+    REQUIRE(variant == variant);
+    REQUIRE_FALSE(variant != variant);
+
     REQUIRE(variant == std::string("hello"));
     REQUIRE_FALSE(variant != std::string("hello"));
 
     REQUIRE(variant != std::string("world"));
     REQUIRE_FALSE(variant == std::string("world"));
 
+    REQUIRE(variant != null);
+    REQUIRE_FALSE(variant == null);
+
     REQUIRE(std::string("hello") == variant);
     REQUIRE_FALSE(std::string("hello") != variant);
 
     REQUIRE(std::string("world") != variant);
     REQUIRE_FALSE(std::string("world") == variant);
+
+    REQUIRE(null != variant);
+    REQUIRE_FALSE(null == variant);
   }
 
   SECTION("IntegerInVariant") {