Avoid copying null in concat, unused + breaks views

[paulocsanz]

We use a few non-null terminated views and it seems String::concat(char*, size_t) breaks those because it always copies the null terminator, but since the function immediately overwrites it, it doesn't need to.

This change makes String::concat support non-null terminated char* without any side effect.

Fixes #8200

[earlephilhower]

Good corner case find!

Can you please add a unit test in tests/host./core/test_string.cpp?

[paulocsanz]

I'm not able to reproduce this anymore, maybe it was a misdiagnose of mine, it was late and I was tired and I ended up fixing a few other bugs in our code simultaneously. I probably stumbled on it by accident and thought the correlation was causality to my crashes.

But I may have indeed reached an edge case that cause a problem because we do not put the stack in the 4kb unused sys stack. So we have a few std::arrays stored in its place, and the one that seemed to trigger the problem was the last one. But it doesn't get to 0x4000000, not even the full 4096 bytes to 0x3FFFF000. So where should memove_P break with a off by one bug like this one?

I've spent quite some time to find a reproducible example but failed. Do you have a suggestion? Sorry for crying wolf (crash) without being sure.

[earlephilhower]

Sorry, I don't have any suggestions. I don't see how the existing code could cause a crash, which is why I was hoping to see an example that fails on the 3.0.1 release but passes with this PR.

That said, I think this PR is correct, though, and current code is reading 1 byte past where it should (the 0-termination is added 2 lines after the memcpy no matter what, no need to memcpy it).

Using host tests lets us use valgrind which checks for out-of-bounds accesses, so even if it doesn't crash it will dump a warning if things are bad.

[paulocsanz]

Nice, so something like this would solve the problem?

void setup() {
  char ch = 'A';
  String str;
  str.concat(&ch, 1);
}

[earlephilhower]

Nice, so something like this would solve the problem?

void setup() {
  char ch = 'A';
  String str;
  str.concat(&ch, 1);
}

More like this...

diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp
index 61931327..7105fa28 100644
--- a/tests/host/core/test_string.cpp
+++ b/tests/host/core/test_string.cpp
@@ -594,3 +594,12 @@ TEST_CASE("String chaining", "[core][String]")
     REQUIRE(static_cast<const void*>(result.c_str()) == static_cast<const void*>(ptr));
   }
 }
+
+TEST_CASE("String concat OOB #8198", "[core][String]")
+{
+    char *p = (char*)malloc(16);
+    memset(p, 'x', 16);
+    String s = "abcd";
+    s.concat(p, 16);
+    REQUIRE(!strcmp(s.c_str(), "abcdxxxxxxxxxxxxxxxx"));
+}

Which generates an error in valgrind on host tests

==2849== Invalid read of size 1
==2849==    at 0x4840270: memcpy@GLIBC_2.2.5 (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==2849==    by 0x1EBA85: memmove (string_fortified.h:40)
==2849==    by 0x1EBA85: String::concat(char const*, unsigned int) (WString.cpp:308)
==2849==    by 0x1AEE10: ____C_A_T_C_H____T_E_S_T____598() (test_string.cpp:603)
==2849==    by 0x1D2CDE: Catch::RunContext::invokeActiveTestCase() [clone .isra.0] (catch.hpp:5473)
==2849==    by 0x1E0C47: Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (catch.hpp:5445)
==2849==    by 0x1E482D: Catch::RunContext::runTest(Catch::TestCase const&) (catch.hpp:5284)
==2849==    by 0x1E7EE8: Catch::Runner::runTests() (catch.hpp:5603)
==2849==    by 0x1E8206: Catch::Session::run() (catch.hpp:5734)
==2849==    by 0x1232F9: run (catch.hpp:5714)
==2849==    by 0x1232F9: main (catch.hpp:9278)
==2849==  Address 0x606b970 is 0 bytes after a block of size 16 alloc'd
==2849==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==2849==    by 0x1AEDAA: ____C_A_T_C_H____T_E_S_T____598() (test_string.cpp:600)
==2849==    by 0x1D2CDE: Catch::RunContext::invokeActiveTestCase() [clone .isra.0] (catch.hpp:5473)
==2849==    by 0x1E0C47: Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (catch.hpp:5445)
==2849==    by 0x1E482D: Catch::RunContext::runTest(Catch::TestCase const&) (catch.hpp:5284)
==2849==    by 0x1E7EE8: Catch::Runner::runTests() (catch.hpp:5603)
==2849==    by 0x1E8206: Catch::Session::run() (catch.hpp:5734)
==2849==    by 0x1232F9: run (catch.hpp:5714)
==2849==    by 0x1232F9: main (catch.hpp:9278)

[paulocsanz]

Done. Thank you for the help.

[earlephilhower]

LGTM now, thanks!

[earlephilhower]

@paulocsanz please add a free(p); to the test to fix the memory loss warning.