Fix String::replace() garbage at end of string (esp8266#5897)

earlephilhower · web-flow · commit 54240d2cc51e · 2019-03-20T06:18:04.000-07:00
* Fix String::replace() Fixes esp8266#5883 and supercedes esp8266#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. Add in test cases from esp8266#5890 as well as some new ones that fail on the unmodified core. * Fix stack smashing error on 64b 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. * 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 thanks to SSO there are no memory allocations, but will help in case someone adds tests later which include longer strings.
diff --git 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/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,65 @@ 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 <new>
+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[64];
+  String *s = (String*)space;
+  memset(space, 0xff, 64);
+  new(s) String;
+  REQUIRE(*s == "");
+  s->~String();
+
+  // 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, 64);
+  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("&euro;");
+  repl(("&euro;"), 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("&yen;");
+  repl(("&yen;"), 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();
 }

Original file line number	Diff line number	Diff line change
`@@ -759,9 +759,10 @@ void String::replace(const String& find, const String& replace) {`
`759`	`759`	`while(index >= 0 && (index = lastIndexOf(find, index)) >= 0) {`
`760`	`760`	`readFrom = wbuffer() + index + find.len();`
`761`	`761`	`memmove(readFrom + diff, readFrom, len() - (readFrom - buffer()));`
`762`		`- setLen(len() + diff);`
`763`		`- wbuffer()[len()] = 0;`
	`762`	`+ int newLen = len() + diff;`
`764`	`763`	`memcpy(wbuffer() + index, replace.buffer(), replace.len());`
	`764`	`+ setLen(newLen);`
	`765`	`+ wbuffer()[newLen] = 0;`
`765`	`766`	`index--;`
`766`	`767`	`}`
`767`	`768`	`}`