Fix String::replace() #5897
Merged
Fix String::replace() #5897
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
devyte
approved these changes
Mar 20, 2019
|
Darn it. I did run "make test" and it passed on my box before submitting. Let me check the code once again, there may be other spots where it's iffy. |
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. Undo setLen actually writing a 0 in memory when called in SSO mode. The semantics of setLen are such that it is called as part of a multi-step process to update a String, and when it is called it does NOT normally write a 0 to the buffer at the length indicated. Doing so in SSO mode and 64b OSes exposed the error, now cleared up.
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 trhanks to SSO there are no memory allocations, but will help in case someone adds tests later which include longer strings.
earlephilhower
changed the title
TD-er
reviewed
Mar 20, 2019
| 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! |
There was a problem hiding this comment.
Always nice to see your own code commented with some instruction "This is NOT how you want to use...." ;)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.