From 2641697e0bf3d11bab593bdbc6e235c15e8c46e7 Mon Sep 17 00:00:00 2001 From: Benoit Blanchon Date: Thu, 27 Feb 2020 11:44:09 +0100 Subject: [PATCH] Fixed incorrect string comparison on some platforms (fixes #1198) --- CHANGELOG.md | 1 + extras/tests/Misc/StringAdapters.cpp | 114 +++++++++++------- extras/tests/Misc/weird_strcmp.hpp | 23 ++++ src/ArduinoJson/Polyfills/safe_strcmp.hpp | 8 +- .../Strings/ArduinoStringAdapter.hpp | 2 +- .../Strings/ConstRamStringAdapter.hpp | 2 +- .../Strings/FlashStringAdapter.hpp | 4 +- .../Strings/SizedFlashStringAdapter.hpp | 5 +- .../Strings/SizedRamStringAdapter.hpp | 4 +- src/ArduinoJson/Strings/StlStringAdapter.hpp | 4 +- 10 files changed, 110 insertions(+), 57 deletions(-) create mode 100644 extras/tests/Misc/weird_strcmp.hpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 68c99db2..4a307d1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ HEAD * Fixed "deprecated-copy" warning on GCC 9 (fixes #1184) * Fixed `MemberProxy::set(char[])` not duplicating the string (issue #1191) * Fixed enums serialized as booleans (issue #1197) +* Fixed incorrect string comparison on some platforms (issue #1198) v6.14.1 (2020-01-27) ------- diff --git a/extras/tests/Misc/StringAdapters.cpp b/extras/tests/Misc/StringAdapters.cpp index 8e6fb6e5..52167d22 100644 --- a/extras/tests/Misc/StringAdapters.cpp +++ b/extras/tests/Misc/StringAdapters.cpp @@ -4,9 +4,11 @@ #include "custom_string.hpp" #include "progmem_emulation.hpp" +#include "weird_strcmp.hpp" #include #include +#include #include #include @@ -17,27 +19,55 @@ TEST_CASE("ConstRamStringAdapter") { SECTION("null") { ConstRamStringAdapter adapter(NULL); - REQUIRE(adapter.compare("bravo") < 0); - REQUIRE(adapter.compare(NULL) == 0); + CHECK(adapter.compare("bravo") < 0); + CHECK(adapter.compare(NULL) == 0); - REQUIRE(adapter.equals(NULL)); - REQUIRE_FALSE(adapter.equals("charlie")); + CHECK(adapter.equals(NULL)); + CHECK_FALSE(adapter.equals("charlie")); - REQUIRE(adapter.size() == 0); + CHECK(adapter.size() == 0); } SECTION("non-null") { ConstRamStringAdapter adapter("bravo"); - REQUIRE(adapter.compare(NULL) > 0); - REQUIRE(adapter.compare("alpha") > 0); - REQUIRE(adapter.compare("bravo") == 0); - REQUIRE(adapter.compare("charlie") < 0); + CHECK(adapter.compare(NULL) > 0); + CHECK(adapter.compare("alpha") > 0); + CHECK(adapter.compare("bravo") == 0); + CHECK(adapter.compare("charlie") < 0); - REQUIRE(adapter.equals("bravo")); - REQUIRE_FALSE(adapter.equals("charlie")); + CHECK(adapter.equals("bravo")); + CHECK_FALSE(adapter.equals("charlie")); - REQUIRE(adapter.size() == 5); + CHECK(adapter.size() == 5); + } +} + +TEST_CASE("SizedRamStringAdapter") { + SECTION("null") { + SizedRamStringAdapter adapter(NULL, 10); + + CHECK(adapter.compare("bravo") < 0); + CHECK(adapter.compare(NULL) == 0); + + CHECK(adapter.equals(NULL)); + CHECK_FALSE(adapter.equals("charlie")); + + CHECK(adapter.size() == 10); + } + + SECTION("non-null") { + SizedRamStringAdapter adapter("bravo", 5); + + CHECK(adapter.compare(NULL) > 0); + CHECK(adapter.compare("alpha") > 0); + CHECK(adapter.compare("bravo") == 0); + CHECK(adapter.compare("charlie") < 0); + + CHECK(adapter.equals("bravo")); + CHECK_FALSE(adapter.equals("charlie")); + + CHECK(adapter.size() == 5); } } @@ -45,27 +75,27 @@ TEST_CASE("FlashStringAdapter") { SECTION("null") { FlashStringAdapter adapter(NULL); - REQUIRE(adapter.compare("bravo") < 0); - REQUIRE(adapter.compare(NULL) == 0); + CHECK(adapter.compare("bravo") < 0); + CHECK(adapter.compare(NULL) == 0); - REQUIRE(adapter.equals(NULL)); - REQUIRE_FALSE(adapter.equals("charlie")); + CHECK(adapter.equals(NULL)); + CHECK_FALSE(adapter.equals("charlie")); - REQUIRE(adapter.size() == 0); + CHECK(adapter.size() == 0); } SECTION("non-null") { FlashStringAdapter adapter = adaptString(F("bravo")); - REQUIRE(adapter.compare(NULL) > 0); - REQUIRE(adapter.compare("alpha") > 0); - REQUIRE(adapter.compare("bravo") == 0); - REQUIRE(adapter.compare("charlie") < 0); + CHECK(adapter.compare(NULL) > 0); + CHECK(adapter.compare("alpha") > 0); + CHECK(adapter.compare("bravo") == 0); + CHECK(adapter.compare("charlie") < 0); - REQUIRE(adapter.equals("bravo")); - REQUIRE_FALSE(adapter.equals("charlie")); + CHECK(adapter.equals("bravo")); + CHECK_FALSE(adapter.equals("charlie")); - REQUIRE(adapter.size() == 5); + CHECK(adapter.size() == 5); } } @@ -73,46 +103,46 @@ TEST_CASE("std::string") { std::string str("bravo"); StlStringAdapter adapter = adaptString(str); - REQUIRE(adapter.compare(NULL) > 0); - REQUIRE(adapter.compare("alpha") > 0); - REQUIRE(adapter.compare("bravo") == 0); - REQUIRE(adapter.compare("charlie") < 0); + CHECK(adapter.compare(NULL) > 0); + CHECK(adapter.compare("alpha") > 0); + CHECK(adapter.compare("bravo") == 0); + CHECK(adapter.compare("charlie") < 0); - REQUIRE(adapter.equals("bravo")); - REQUIRE_FALSE(adapter.equals("charlie")); + CHECK(adapter.equals("bravo")); + CHECK_FALSE(adapter.equals("charlie")); - REQUIRE(adapter.size() == 5); + CHECK(adapter.size() == 5); } TEST_CASE("custom_string") { custom_string str("bravo"); StlStringAdapter adapter = adaptString(str); - REQUIRE(adapter.compare(NULL) > 0); - REQUIRE(adapter.compare("alpha") > 0); - REQUIRE(adapter.compare("bravo") == 0); - REQUIRE(adapter.compare("charlie") < 0); + CHECK(adapter.compare(NULL) > 0); + CHECK(adapter.compare("alpha") > 0); + CHECK(adapter.compare("bravo") == 0); + CHECK(adapter.compare("charlie") < 0); - REQUIRE(adapter.equals("bravo")); - REQUIRE_FALSE(adapter.equals("charlie")); + CHECK(adapter.equals("bravo")); + CHECK_FALSE(adapter.equals("charlie")); - REQUIRE(adapter.size() == 5); + CHECK(adapter.size() == 5); } TEST_CASE("IsString") { SECTION("std::string") { - REQUIRE(IsString::value == true); + CHECK(IsString::value == true); } SECTION("basic_string") { - REQUIRE(IsString >::value == false); + CHECK(IsString >::value == false); } SECTION("custom_string") { - REQUIRE(IsString::value == true); + CHECK(IsString::value == true); } SECTION("const __FlashStringHelper*") { - REQUIRE(IsString::value == true); + CHECK(IsString::value == true); } } diff --git a/extras/tests/Misc/weird_strcmp.hpp b/extras/tests/Misc/weird_strcmp.hpp new file mode 100644 index 00000000..3dbc0cf2 --- /dev/null +++ b/extras/tests/Misc/weird_strcmp.hpp @@ -0,0 +1,23 @@ +#include + +// Issue #1198: strcmp() implementation that returns a value larger than 8-bit + +namespace ARDUINOJSON_NAMESPACE { +int strcmp(const char* a, const char* b) { + int result = ::strcmp(a, b); + if (result > 0) + return 2147483647; + if (result < 0) + return -214748364; + return 0; +} + +int strncmp(const char* a, const char* b, size_t n) { + int result = ::strncmp(a, b, n); + if (result > 0) + return 2147483647; + if (result < 0) + return -214748364; + return 0; +} +} // namespace ARDUINOJSON_NAMESPACE diff --git a/src/ArduinoJson/Polyfills/safe_strcmp.hpp b/src/ArduinoJson/Polyfills/safe_strcmp.hpp index 135f4957..1ae8f552 100644 --- a/src/ArduinoJson/Polyfills/safe_strcmp.hpp +++ b/src/ArduinoJson/Polyfills/safe_strcmp.hpp @@ -10,23 +10,23 @@ namespace ARDUINOJSON_NAMESPACE { -inline int8_t safe_strcmp(const char* a, const char* b) { +inline int safe_strcmp(const char* a, const char* b) { if (a == b) return 0; if (!a) return -1; if (!b) return 1; - return static_cast(strcmp(a, b)); + return strcmp(a, b); } -inline int8_t safe_strncmp(const char* a, const char* b, size_t n) { +inline int safe_strncmp(const char* a, const char* b, size_t n) { if (a == b) return 0; if (!a) return -1; if (!b) return 1; - return static_cast(strncmp(a, b, n)); + return strncmp(a, b, n); } } // namespace ARDUINOJSON_NAMESPACE diff --git a/src/ArduinoJson/Strings/ArduinoStringAdapter.hpp b/src/ArduinoJson/Strings/ArduinoStringAdapter.hpp index c2e39d84..dab1d251 100644 --- a/src/ArduinoJson/Strings/ArduinoStringAdapter.hpp +++ b/src/ArduinoJson/Strings/ArduinoStringAdapter.hpp @@ -31,7 +31,7 @@ class ArduinoStringAdapter { return !_str->c_str(); } - int8_t compare(const char* other) const { + int compare(const char* other) const { // Arduino's String::c_str() can return NULL const char* me = _str->c_str(); return safe_strcmp(me, other); diff --git a/src/ArduinoJson/Strings/ConstRamStringAdapter.hpp b/src/ArduinoJson/Strings/ConstRamStringAdapter.hpp index 7cc95390..7014079d 100644 --- a/src/ArduinoJson/Strings/ConstRamStringAdapter.hpp +++ b/src/ArduinoJson/Strings/ConstRamStringAdapter.hpp @@ -17,7 +17,7 @@ class ConstRamStringAdapter { public: ConstRamStringAdapter(const char* str = 0) : _str(str) {} - int8_t compare(const char* other) const { + int compare(const char* other) const { return safe_strcmp(_str, other); } diff --git a/src/ArduinoJson/Strings/FlashStringAdapter.hpp b/src/ArduinoJson/Strings/FlashStringAdapter.hpp index 673a2b31..8d78c1d6 100644 --- a/src/ArduinoJson/Strings/FlashStringAdapter.hpp +++ b/src/ArduinoJson/Strings/FlashStringAdapter.hpp @@ -15,14 +15,14 @@ class FlashStringAdapter { public: FlashStringAdapter(const __FlashStringHelper* str) : _str(str) {} - int8_t compare(const char* other) const { + int compare(const char* other) const { if (!other && !_str) return 0; if (!_str) return -1; if (!other) return 1; - return int8_t(-strcmp_P(other, reinterpret_cast(_str))); + return -strcmp_P(other, reinterpret_cast(_str)); } bool equals(const char* expected) const { diff --git a/src/ArduinoJson/Strings/SizedFlashStringAdapter.hpp b/src/ArduinoJson/Strings/SizedFlashStringAdapter.hpp index 2d77b390..8e418b70 100644 --- a/src/ArduinoJson/Strings/SizedFlashStringAdapter.hpp +++ b/src/ArduinoJson/Strings/SizedFlashStringAdapter.hpp @@ -15,15 +15,14 @@ class SizedFlashStringAdapter { SizedFlashStringAdapter(const __FlashStringHelper* str, size_t sz) : _str(str), _size(sz) {} - int8_t compare(const char* other) const { + int compare(const char* other) const { if (!other && !_str) return 0; if (!_str) return -1; if (!other) return 1; - return int8_t( - -strncmp_P(other, reinterpret_cast(_str), _size)); + return -strncmp_P(other, reinterpret_cast(_str), _size); } bool equals(const char* expected) const { diff --git a/src/ArduinoJson/Strings/SizedRamStringAdapter.hpp b/src/ArduinoJson/Strings/SizedRamStringAdapter.hpp index c54769fe..1c16ffd5 100644 --- a/src/ArduinoJson/Strings/SizedRamStringAdapter.hpp +++ b/src/ArduinoJson/Strings/SizedRamStringAdapter.hpp @@ -16,8 +16,8 @@ class SizedRamStringAdapter { public: SizedRamStringAdapter(const char* str, size_t n) : _str(str), _size(n) {} - int8_t compare(const char* other) const { - return safe_strncmp(_str, other, _size) == 0; + int compare(const char* other) const { + return safe_strncmp(_str, other, _size); } bool equals(const char* expected) const { diff --git a/src/ArduinoJson/Strings/StlStringAdapter.hpp b/src/ArduinoJson/Strings/StlStringAdapter.hpp index c5eef2be..37f857bf 100644 --- a/src/ArduinoJson/Strings/StlStringAdapter.hpp +++ b/src/ArduinoJson/Strings/StlStringAdapter.hpp @@ -30,10 +30,10 @@ class StlStringAdapter { return false; } - int8_t compare(const char* other) const { + int compare(const char* other) const { if (!other) return 1; - return static_cast(_str->compare(other)); + return _str->compare(other); } bool equals(const char* expected) const {