Fix String creation and concat issues found by Valgrind #4955
Conversation
When a string is concatted to itself, the pointer to its c_str can change
due to realloc(). This would invalidate the passed-in pointer being
concatted, and cause a use-after-free error. Special case this to avoid
the issue. Now "a += a;" works properly.
Also use sprintf(%{l}d) instead of non-POSIX ltoa/itoa calls to construct a
string from a signed number (in base 10 only). The non-posix versions don't
handle INT_MIN properly on either host_tests or on the ESP8266.
devyte
added
component: core
staged-for-release
type: bug
and removed
staged-for-release
labels
| return 1; | ||
| if (!reserve(newlen)) | ||
| return 0; | ||
| memcpy(s.buffer + len, s.buffer, len); |
There was a problem hiding this comment.
If len includes the null term, then this will start copying after the first string's null term. If not, then this doesn't assure a null term result.
There was a problem hiding this comment.
Len doesn't include the null. But this sequence does guarantee a 0-termination because reserve 0-fills the buffer on allocation...
https://github.com/earlephilhower/Arduino/blob/713faf8fe665d5e50f4b10618fdf584853d26052/cores/esp8266/WString.cpp#L161
There was a problem hiding this comment.
How very inefficient... Every time I look at this implementation I want to migrate to a std::string-based one.
In that case, approving.
| // Special case if we're concatting ourself (s += s;) since we may end up | ||
| // realloc'ing the buffer and moving s.buffer in the method called | ||
| if (&s == this) { | ||
| unsigned int newlen = 2 * len; |
There was a problem hiding this comment.
if len includes the null term, then this is 1 byte too big. If not, then this is ok.
There was a problem hiding this comment.
Len's only the actual character count. String is a mishmash of Pascal and C strings, with a dash of what-the-heck-were-they-thinking added, just for fun.
As @devyte noticed, PR esp8266#4955 has an issue when you catenate a string to itself and the string used to hold a longer value because it does not explicitly 0-terminate the resulting string. If the string was extended, however, reserve() would 0-terminate by default. Always terminate the result of `s += s;` now.
As @devyte noticed, PR #4955 has an issue when you catenate a string to itself and the string used to hold a longer value because it does not explicitly 0-terminate the resulting string. If the string was extended, however, reserve() would 0-terminate by default. Always terminate the result of `s += s;` now.
When a string is concatted to itself, the pointer to its c_str can change
due to realloc(). This would invalidate the passed-in pointer being
concatted, and cause a use-after-free error. Special case this to avoid
the issue. Now "a += a;" works properly.
Also use sprintf(%{l}d) instead of non-POSIX ltoa/itoa calls to construct a
string from a signed number (in base 10 only). The non-posix versions don't
handle INT_MIN properly on either host_tests or on the ESP8266.