Fix Small String Optimization misplaced 0 terminator #5890

TD-er · 2019-03-18T00:04:18Z

A fix for some issue introduced in PR #5690
See discussion in #5883

A fix for some issue introduced in PR esp8266#5690 See discussion in esp8266#5883

devyte · 2019-03-18T14:57:28Z

cores/esp8266/WString.cpp

@@ -161,11 +162,11 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) {
            return 1;
        } else { // if bufptr && !sso()
            // Using bufptr, need to shrink into sso_buff
-            char temp[sizeof(sso_buf)];
+            char temp[sizeof(sso_buf)] = {0};


How about the following, which would be more efficient:

char temp[sizeof(sso_buf)]; memcpy(temp, buffer(), maxStrLen); free(wbuffer()); setSSO(true); memcpy(wbuffer(), temp, maxStrLen); wbuffer()[maxStrLen] = 0;

I guess copying less may improve it a bit theoretically, but all other calls to memcpy regarding the sso_buf do copy the entire buffer.
So for the sake of consistency, it may be wise to have them the same.
The reason it wasn't , did trigger me to have a look at that line.

I don't know how much impact the zeroing has at the time of construction (on the stack).

But since we're also setting it to all zeros in the init(), it may be a bit more than needed.
N.B. That zero'ing in the init is really needed.

It seems only setting a string to "" would have any different behavior in this patch as-is, vs. @devyte's which would explicitly write a 0 at maxstrlen. I think his is correct (but again, I'm having trouble seeing where this actually changes behavior...)

earlephilhower

Thanks for the PR, but I'm not sure I understand the logic in some places...

earlephilhower · 2019-03-18T21:52:33Z

tests/host/core/test_string.cpp

+  s = "0123456789abcde";
+  s = s.substring(s.indexOf('a'));
+  REQUIRE(s == "abcde");
+  REQUIRE(s.length() == 5);


These and the prior added test pass on the unpatched core. Were they supposed to fail?

I was hoping there would be something that fails like your app code did, but passes once the patch is applied.

I did not test the tests myself. These were merely added to see how often they fail, but I guess it also depends on how the data on the stack is being allocated.
Does the test environment have some debug compile options which may set some allocated arrays to 0 first? (would perhaps be not the best options for a test environment I guess)

I was thinking about this, but need @devyte's C++ internals help. Basically you want to get a RAM buffer of 16 bytes, fill it with 0xff, and then coerce a pointer to that buffer into a (String*). The bugger is you need to call the constructor on it, and I've no idea if that's possible explicitly in C++.

-edit-
StackOverflow seems to believe it's supported:
https://stackoverflow.com/questions/2494471/c-is-it-possible-to-call-a-constructor-directly-without-new

earlephilhower · 2019-03-18T22:00:18Z

cores/esp8266/WString.cpp

@@ -129,6 +129,7 @@ String::~String() {
 // /*********************************************/

 inline void String::init(void) {
+    memset(sso_buf, 0, sizeof(sso_buf));


I can't see why the initial zeroing in ::init is needed, honestly. Can you explain? Once you apply a string of >12 chars to the String, a pointer and 2 length fields will be written over the 0s you wrote in ::init. At some later point you assign a 1-char string the SSO better write 0 to sso_buff[1] or it'll go poorly...

I did test it without this memset and then the garbled texts re-appeared in the use case I showed in the issue.
The main reason I placed this memset in the init, is because that's the only point where I knew for sure that any data present would not be worth keeping.

Immediately after setting this buffer to 0's, the mode is switched to non-SSO.
So then all calls made in init are only on the _ptr struct.
N.B. this struct is then also set to contain 0's, but it does not overlap all the space occupied by sso_buf

In the constructor where I had the issues, only the copy is called after init.
This calls reserve, which calls changeBuffer.

In changeBuffer we enter this part, since the ptr.buff is set to 0 in init:

Arduino/cores/esp8266/WString.cpp

Lines 156 to 162 in db88c82

unsigned char String::changeBuffer(unsigned int maxStrLen) {

// Can we use SSO here to avoid allocation?

if (maxStrLen < sizeof(sso_buf)) {

if (sso() || !buffer()) {

// Already using SSO, nothing to do

setSSO(true);

return 1;

So only SSO is set and nothing else.
Since nothing has changed to the SSO buffer, but SSO is now active, a call to len() may return something other than 0 (it is calling strlen(sso_buf) which may return anything)

So the first byte of the buffer is not set to 0 and thus the String object may contain garbage and it is up to the compiler how this union is aligned.
If the pointer of _ptr is at the first byte of the sso_buf, then it should already be a zero, but apparently the Windows build is not locating the _prt struct at the same address as sso_buf.

By wiping as much as possible to 0 in this union struct, it is guaranteed it will contain zeroes at the start.
Also I guess it could differ between builds where this union is located and I don't know what all possible offsets are between the start of _ptr and sso_buf.

So that's why I think it is needed to have the sso_buf initialized to 0.
Does that make any sense?

AMD64 compiles are going to align things somewhat differently than X86_32 ones do (the pointer, for example, is gonna be 2x the size...). I don't know if Windows supports it w/the mingw(?) toolchain, but there is a 32-bit compile mode in the makefile which should help get closer to on-board alignment.

I'm not sure I fully follow, but my point is in the general case the sequence you've got is not guaranteed to happen, and there's possibility for the early bytes of the union to contain non-0s after you've called ::init and when switching. 0-ing out at ::init is safe, but I think it's unneeded.

I think the main problem is you found that when a SSO string was copied into the struct it did not get explicitly terminated w/a \0. Your patch does fix that, later on in the SSO assignment changeBuffer. My gut says that dropping this memset but leaving everything else untouched won't change the behavior.

Well, leaving out this memset did not fix the issue (tested it).
I'm also not 100% convinced that whatever this init does fix will cover all situations, but at least it does something.
Maybe the suggestion I made yesterday does cover all and then this part in the init can be removed?

I don't understand what you tried to explain with the difference on 64-bit and 32 bit platforms.
Do the compilers generate different code even though they compile for the same 32 bit platform?
I know the size_t, ptrdiff_t and pointers have different sizes when compiling for 64 bit or 32 bit platforms, but when they compile for the same 32 bit platform they should align structures in memory the same, right?
Otherwise we might have a problem when serializing structs.

I was talking about the host testing. It can run either 32-bit or 64-bit on your system, depending on the compilers available and make options. The gcc-xtensa binaries should produce the exact same code under either host OS, of course.

earlephilhower · 2019-03-18T22:01:18Z

cores/esp8266/WString.cpp

@@ -161,11 +162,11 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) {
            return 1;
        } else { // if bufptr && !sso()
            // Using bufptr, need to shrink into sso_buff
-            char temp[sizeof(sso_buf)];
+            char temp[sizeof(sso_buf)] = {0};


It seems only setting a string to "" would have any different behavior in this patch as-is, vs. @devyte's which would explicitly write a 0 at maxstrlen. I think his is correct (but again, I'm having trouble seeing where this actually changes behavior...)

TD-er · 2019-03-19T08:20:20Z

I was thinking, maybe the right moment to wipe the whole buffer (or the first byte) is when switching from non-SSO mode to SSO.

Current implementation:

Arduino/cores/esp8266/WString.h

Line 266 in db88c82

inline void setSSO(bool sso) { sso_buf[SSOSIZE - 1] = sso ? 0x00 : 0xff; }

New implementation could then be something like:

inline void setSSO(bool sso) { 
  if (sso() == sso) return; 
  if (sso) { 
    memset(sso_buf, 0, SSOSIZE); 
  } else {
    sso_buf[SSOSIZE - 1] = 0xff;
  }
}

earlephilhower · 2019-03-20T00:25:41Z

Here is a test which shows failure on the main branch. Probably can unroll the repl() call, I was just lazy and wanted to find something which failed before but passes now.

#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[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("{E}");
  repl(("{E}"), euro, *s, useURLencode);
  REQUIRE(*s == "€");
  memset(space, 0xff, 16);
  new(s) String("&euro;");
  repl(("&euro;"), 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("&yen;");
  repl(("&yen;"), yen, *s, useURLencode);
  REQUIRE(*s == "¥");

  memset(space, 0xff, 16);
  new(s) String("%ssid%");
  repl(("%ssid%"), "MikroTik", *s, useURLencode);
  REQUIRE(*s == "MikroTik");
  memset(space, 0xff, 16);
  new(s) String("%sysname%");
  repl(("%sysname%"), "CO2_defect", *s, useURLencode);
  REQUIRE(*s == "CO2_defect");

}

earlephilhower · 2019-03-20T00:30:14Z

Running the above test with master branch with only the memset 0 of *this in ::init passes. So all other changes in the PR aren't material. And it also feels like this memset 0 is treating the symptom and not the root cause...

earlephilhower · 2019-03-20T00:51:08Z

I think the real culprit is in String::replace after tracing this through on the host in GDB. I think I was playing fast and loose with len(), which isn't separately stored in SSO mode, and needs to be saved before doing internal ops if you're going to change the internal SSO length...

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. 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 esp8266#5890 as well as some new ones that fail on the unmodified core.

earlephilhower · 2019-03-20T01:22:48Z

@TD-er, please try #5897 in your real system. That was the underlying cause, me using len() while in the middle of a string expansion operation in SSO mode.

TD-er · 2019-03-20T08:23:21Z

@TD-er, please try #5897 in your real system. That was the underlying cause, me using len() while in the middle of a string expansion operation in SSO mode.

It works, the unit is now telling me it is connected to the "MikroTik" AP again and not some garbage one ;)

* Fix String::replace() 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. Add in test cases from #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.

earlephilhower · 2019-03-20T13:18:48Z

Thanks for the update. I've merged the other PR and will close this one...

Fix Small String Optimization misplaced 0 terminator

db88c82

A fix for some issue introduced in PR esp8266#5690 See discussion in esp8266#5883

devyte requested changes Mar 18, 2019

View reviewed changes

earlephilhower requested changes Mar 18, 2019

View reviewed changes

earlephilhower mentioned this pull request Mar 20, 2019

Fix String::replace() #5897

Merged

earlephilhower closed this Mar 20, 2019

TD-er deleted the bugfix/String_SSO branch March 20, 2019 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Small String Optimization misplaced 0 terminator #5890

Fix Small String Optimization misplaced 0 terminator #5890

TD-er commented Mar 18, 2019

devyte Mar 18, 2019

TD-er Mar 18, 2019

earlephilhower Mar 18, 2019

earlephilhower left a comment

earlephilhower Mar 18, 2019

TD-er Mar 19, 2019

earlephilhower Mar 19, 2019

earlephilhower Mar 18, 2019

TD-er Mar 19, 2019

earlephilhower Mar 19, 2019

TD-er Mar 20, 2019

earlephilhower Mar 20, 2019

earlephilhower Mar 18, 2019

TD-er commented Mar 19, 2019 •

edited

Loading

earlephilhower commented Mar 20, 2019

earlephilhower commented Mar 20, 2019

earlephilhower commented Mar 20, 2019

earlephilhower commented Mar 20, 2019

TD-er commented Mar 20, 2019

earlephilhower commented Mar 20, 2019

	unsigned char String::changeBuffer(unsigned int maxStrLen) {
	// Can we use SSO here to avoid allocation?
	if (maxStrLen < sizeof(sso_buf)) {
	if (sso() \|\| !buffer()) {
	// Already using SSO, nothing to do
	setSSO(true);
	return 1;

Fix Small String Optimization misplaced 0 terminator #5890

Fix Small String Optimization misplaced 0 terminator #5890

Conversation

TD-er commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earlephilhower left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TD-er commented Mar 19, 2019 • edited Loading

earlephilhower commented Mar 20, 2019

earlephilhower commented Mar 20, 2019

earlephilhower commented Mar 20, 2019

earlephilhower commented Mar 20, 2019

TD-er commented Mar 20, 2019

earlephilhower commented Mar 20, 2019

TD-er commented Mar 19, 2019 •

edited

Loading