Fix String.replace overlapping strcpy (esp8266#5966)

* Fix String.replace overlapping strcpy Fixes esp8266#5949 Adds a test from the issue above and fixes the problem valgrind found. Additional pathological memcpy->memmove fixes
gliadis · Apr 10, 2019 · 9712170 · 9712170
1 parent 4b596d8
commit 9712170
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 6 deletions.
diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp
@@ -237,7 +237,7 @@ void String::move(String &rhs) {
     }
     if (rhs.sso()) {
         setSSO(true);
-        memcpy(sso_buf, rhs.sso_buf, sizeof(sso_buf));
+        memmove(sso_buf, rhs.sso_buf, sizeof(sso_buf));
     } else {
         setSSO(false);
         setBuffer(rhs.wbuffer());
@@ -730,21 +730,21 @@ void String::replace(const String& find, const String& replace) {
     char *foundAt;
     if(diff == 0) {
         while((foundAt = strstr(readFrom, find.buffer())) != NULL) {
-            memcpy(foundAt, replace.buffer(), replace.len());
+            memmove(foundAt, replace.buffer(), replace.len());
             readFrom = foundAt + replace.len();
         }
     } else if(diff < 0) {
         char *writeTo = wbuffer();
         while((foundAt = strstr(readFrom, find.buffer())) != NULL) {
             unsigned int n = foundAt - readFrom;
-            memcpy(writeTo, readFrom, n);
+            memmove(writeTo, readFrom, n);
             writeTo += n;
-            memcpy(writeTo, replace.buffer(), replace.len());
+            memmove(writeTo, replace.buffer(), replace.len());
             writeTo += replace.len();
             readFrom = foundAt + find.len();
             setLen(len() + diff);
         }
-        strcpy(writeTo, readFrom);
+        memmove(writeTo, readFrom, strlen(readFrom)+1);
     } else {
         unsigned int size = len(); // compute size needed for result
         while((foundAt = strstr(readFrom, find.buffer())) != NULL) {
@@ -760,7 +760,7 @@ void String::replace(const String& find, const String& replace) {
             readFrom = wbuffer() + index + find.len();
             memmove(readFrom + diff, readFrom, len() - (readFrom - buffer()));
 	    int newLen = len() + diff;
-            memcpy(wbuffer() + index, replace.buffer(), replace.len());
+            memmove(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
@@ -428,3 +428,15 @@ TEST_CASE("String SSO handles junk in memory", "[core][String]")
   REQUIRE(*s == "CO2_defect");
   s->~String();
 }
+
+
+TEST_CASE("Issue #5949 - Overlapping src/dest in replace", "[core][String]")
+{
+  String blah = "blah";
+  blah.replace("xx", "y");
+  REQUIRE(blah == "blah");
+  blah.replace("x", "yy");
+  REQUIRE(blah == "blah");
+  blah.replace(blah, blah);
+  REQUIRE(blah == "blah");
+}