From 8f548be09ad729fb53c45b1a6798b75971d83b43 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 19 Mar 2019 18:17:23 -0700 Subject: [PATCH 1/3] Fix String::replace() and clean up SSO setLen Fixes #5883 and supercedes #5890 The replace() function was using len() while in the middle of buffer operations. In SSO mode len() is not stored separately and is a call to strlen(), which may not be legal if you're in the middle of overwriting the SSO buffer, as was the case in ::replace when the replacement string was longer than the find string. This caused potential garbage at the end of the string when accessed. Instead, just cache the length in a local while doing the operation. Also make setLength() explicitly write a 0 in the SSO buffer at the specific offset. It's probably not needed, but is safe to do and makes logical sense. Add in test cases from #5890 as well as some new ones that fail on the unmodified core. --- cores/esp8266/WString.cpp | 5 +-- cores/esp8266/WString.h | 2 +- tests/host/core/test_string.cpp | 60 +++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 4d4fa3bf21..ad826976f1 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -759,9 +759,10 @@ void String::replace(const String& find, const String& replace) { while(index >= 0 && (index = lastIndexOf(find, index)) >= 0) { readFrom = wbuffer() + index + find.len(); memmove(readFrom + diff, readFrom, len() - (readFrom - buffer())); - setLen(len() + diff); - wbuffer()[len()] = 0; + int newLen = len() + diff; memcpy(wbuffer() + index, replace.buffer(), replace.len()); + setLen(newLen); + wbuffer()[newLen] = 0; index--; } } diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index aecf9b4a62..209d9ce3ae 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -264,7 +264,7 @@ class String { inline unsigned int len() const { return sso() ? strlen(sso_buf) : ptr.len; } inline unsigned int capacity() const { return sso() ? SSOSIZE - 1 : ptr.cap; } inline void setSSO(bool sso) { sso_buf[SSOSIZE - 1] = sso ? 0x00 : 0xff; } - inline void setLen(int len) { if (!sso()) ptr.len = len; } + inline void setLen(int len) { if (!sso()) ptr.len = len; else sso_buf[len] = 0; } inline void setCapacity(int cap) { if (!sso()) ptr.cap = cap; } inline void setBuffer(char *buff) { if (!sso()) ptr.buff = buff; } // Buffer accessor functions diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp index c2ef68025f..b291ef5eff 100644 --- a/tests/host/core/test_string.cpp +++ b/tests/host/core/test_string.cpp @@ -83,6 +83,12 @@ TEST_CASE("String constructors", "[core][String]") REQUIRE(ssh == "3.14159_abcd"); String flash = (F("hello from flash")); REQUIRE(flash == "hello from flash"); + const char textarray[6] = {'h', 'e', 'l', 'l', 'o', 0}; + String hello(textarray); + REQUIRE(hello == "hello"); + String hello2; + hello2 = textarray; + REQUIRE(hello2 == "hello"); } TEST_CASE("String concantenation", "[core][String]") @@ -360,4 +366,58 @@ TEST_CASE("String SSO works", "[core][String]") REQUIRE(s == "0123456789abcdefghijklmnopqrstu"); REQUIRE(s.length() == 31); } + s = "0123456789abcde"; + s = s.substring(s.indexOf('a')); + REQUIRE(s == "abcde"); + REQUIRE(s.length() == 5); +} + +#include +void repl(const String& key, const String& val, String& s, boolean useURLencode) +{ + s.replace(key, val); +} + + +TEST_CASE("String SSO handles junk in memory", "[core][String]") +{ + // We fill the SSO space with garbage then construct an object in it and check consistency + // This is NOT how you want to use Strings outside of this testing! + unsigned char space[16]; + String *s = (String*)space; + memset(space, 0xff, 16); + new(s) String; + REQUIRE(*s == ""); + + // Tests from #5883 + bool useURLencode = false; + const char euro[4] = {(char)0xe2, (char)0x82, (char)0xac, 0}; // Unicode euro symbol + const char yen[3] = {(char)0xc2, (char)0xa5, 0}; // Unicode yen symbol + + memset(space, 0xff, 16); + new(s) String("%ssid%"); + repl(("%ssid%"), "MikroTik", *s, useURLencode); + REQUIRE(*s == "MikroTik"); + + memset(space, 0xff, 16); + new(s) String("{E}"); + repl(("{E}"), euro, *s, useURLencode); + REQUIRE(*s == "€"); + memset(space, 0xff, 16); + new(s) String("€"); + repl(("€"), euro, *s, useURLencode); + REQUIRE(*s == "€"); + memset(space, 0xff, 16); + new(s) String("{Y}"); + repl(("{Y}"), yen, *s, useURLencode); + REQUIRE(*s == "¥"); + memset(space, 0xff, 16); + new(s) String("¥"); + repl(("¥"), yen, *s, useURLencode); + REQUIRE(*s == "¥"); + + memset(space, 0xff, 16); + new(s) String("%sysname%"); + repl(("%sysname%"), "CO2_defect", *s, useURLencode); + REQUIRE(*s == "CO2_defect"); } From f3522c605762ee815e770d299f4631038372ca22 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 19 Mar 2019 20:19:13 -0700 Subject: [PATCH 2/3] Fix stack smashing error on 64b, undo setLen 0 When pointers are 8 bytes long, the size of a String is larger than 16 chars. Increase the allocated array we're using in the test to avoid a "stack smashing" error. Undo setLen actually writing a 0 in memory when called in SSO mode. The semantics of setLen are such that it is called as part of a multi-step process to update a String, and when it is called it does NOT normally write a 0 to the buffer at the length indicated. Doing so in SSO mode and 64b OSes exposed the error, now cleared up. --- cores/esp8266/WString.h | 2 +- tests/host/core/test_string.cpp | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 209d9ce3ae..aecf9b4a62 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -264,7 +264,7 @@ class String { inline unsigned int len() const { return sso() ? strlen(sso_buf) : ptr.len; } inline unsigned int capacity() const { return sso() ? SSOSIZE - 1 : ptr.cap; } inline void setSSO(bool sso) { sso_buf[SSOSIZE - 1] = sso ? 0x00 : 0xff; } - inline void setLen(int len) { if (!sso()) ptr.len = len; else sso_buf[len] = 0; } + inline void setLen(int len) { if (!sso()) ptr.len = len; } inline void setCapacity(int cap) { if (!sso()) ptr.cap = cap; } inline void setBuffer(char *buff) { if (!sso()) ptr.buff = buff; } // Buffer accessor functions diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp index b291ef5eff..a282cb246c 100644 --- a/tests/host/core/test_string.cpp +++ b/tests/host/core/test_string.cpp @@ -383,9 +383,9 @@ TEST_CASE("String SSO handles junk in memory", "[core][String]") { // We fill the SSO space with garbage then construct an object in it and check consistency // This is NOT how you want to use Strings outside of this testing! - unsigned char space[16]; + unsigned char space[64]; String *s = (String*)space; - memset(space, 0xff, 16); + memset(space, 0xff, 64); new(s) String; REQUIRE(*s == ""); @@ -394,29 +394,29 @@ TEST_CASE("String SSO handles junk in memory", "[core][String]") const char euro[4] = {(char)0xe2, (char)0x82, (char)0xac, 0}; // Unicode euro symbol const char yen[3] = {(char)0xc2, (char)0xa5, 0}; // Unicode yen symbol - memset(space, 0xff, 16); + memset(space, 0xff, 64); new(s) String("%ssid%"); repl(("%ssid%"), "MikroTik", *s, useURLencode); REQUIRE(*s == "MikroTik"); - memset(space, 0xff, 16); + memset(space, 0xff, 64); new(s) String("{E}"); repl(("{E}"), euro, *s, useURLencode); REQUIRE(*s == "€"); - memset(space, 0xff, 16); + memset(space, 0xff, 64); new(s) String("€"); repl(("€"), euro, *s, useURLencode); REQUIRE(*s == "€"); - memset(space, 0xff, 16); + memset(space, 0xff, 64); new(s) String("{Y}"); repl(("{Y}"), yen, *s, useURLencode); REQUIRE(*s == "¥"); - memset(space, 0xff, 16); + memset(space, 0xff, 64); new(s) String("¥"); repl(("¥"), yen, *s, useURLencode); REQUIRE(*s == "¥"); - memset(space, 0xff, 16); + memset(space, 0xff, 64); new(s) String("%sysname%"); repl(("%sysname%"), "CO2_defect", *s, useURLencode); REQUIRE(*s == "CO2_defect"); From 92eab01fbbdf69041eab7f7bcfd66ddd2b2dce41 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Tue, 19 Mar 2019 20:24:47 -0700 Subject: [PATCH 3/3] Manually call destructor in test Just for clarity, manually call the destructor for the Strings() that are "placement new'd" in the String tests. It is a no-op for the existing test, since trhanks to SSO there are no memory allocations, but will help in case someone adds tests later which include longer strings. --- tests/host/core/test_string.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp index a282cb246c..9ffd6d4162 100644 --- a/tests/host/core/test_string.cpp +++ b/tests/host/core/test_string.cpp @@ -388,6 +388,7 @@ TEST_CASE("String SSO handles junk in memory", "[core][String]") memset(space, 0xff, 64); new(s) String; REQUIRE(*s == ""); + s->~String(); // Tests from #5883 bool useURLencode = false; @@ -398,26 +399,32 @@ TEST_CASE("String SSO handles junk in memory", "[core][String]") new(s) String("%ssid%"); repl(("%ssid%"), "MikroTik", *s, useURLencode); REQUIRE(*s == "MikroTik"); + s->~String(); memset(space, 0xff, 64); new(s) String("{E}"); repl(("{E}"), euro, *s, useURLencode); REQUIRE(*s == "€"); + s->~String(); memset(space, 0xff, 64); new(s) String("€"); repl(("€"), euro, *s, useURLencode); REQUIRE(*s == "€"); + s->~String(); memset(space, 0xff, 64); new(s) String("{Y}"); repl(("{Y}"), yen, *s, useURLencode); REQUIRE(*s == "¥"); + s->~String(); memset(space, 0xff, 64); new(s) String("¥"); repl(("¥"), yen, *s, useURLencode); REQUIRE(*s == "¥"); + s->~String(); memset(space, 0xff, 64); new(s) String("%sysname%"); repl(("%sysname%"), "CO2_defect", *s, useURLencode); REQUIRE(*s == "CO2_defect"); + s->~String(); }