Skip to content

Commit

Permalink
Fix String::replace() garbage at end of string (esp8266#5897)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
earlephilhower authored Mar 20, 2019
1 parent 64e30b2 commit 54240d2
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 2 deletions.
5 changes: 3 additions & 2 deletions cores/esp8266/WString.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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--;
}
}
Expand Down
67 changes: 67 additions & 0 deletions tests/host/core/test_string.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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]")
Expand Down Expand Up @@ -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();
}

0 comments on commit 54240d2

Please sign in to comment.