Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork137
Strings without null-termination#97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cvwillegen commentedApr 11, 2020
Can this please be merged? I'm running into problems with this, as I would like to use string with embedded NUL chars in them. When copying a String with a NUL char in it, the memory after the NUL bytes is not being copied, so the rest of that String is uninitialised memory. Here's sample code showing the error: Output of this sample: |
cvwillegen commentedMay 12, 2020
@matthijskooijman How can I build this and run it on my own Arduino's? As you might recall, I added#112 and would like to create a patch to fix it. But then I need to compile this code myself in order to fix it. How does one do that? |
matthijskooijman commentedMay 12, 2020
@cvwillegen You should be able to clone this repository (and then the right branch) into Then, it is probably useful to change the If you then restart the Arduino IDE, you should the AVR boards twice in the boards menu. If you select the right one, it will use the code from git rather than the official release. Is that enough info? |
aentinger commentedNov 27, 2020
@matthijskooijman can you please rebase? I'd like to see the CI unit tests run, probably the tests need some fixing. |
721ed20 to9a604e7Comparecodecov-io commentedDec 31, 2020
Codecov Report
@@ Coverage Diff @@## master #97 +/- ##==========================================- Coverage 96.41% 96.07% -0.35%========================================== Files 14 14 Lines 837 840 +3 ========================================== Hits 807 807- Misses 30 33 +3
Continue to review full report at Codecov.
|
matthijskooijman commentedDec 31, 2020
@aentinger I finally got around to having another look at this, I see you already did a rebase. I just did another force push, which:
With that, the testcases succeed again. I think this could be merged as-is (after squashing the fixup), but I also want to look at:
Ideally, we'd do 1. and 2. before merging this, 3. is probably more tricky and might be left for later. |
cvwillegen commentedDec 31, 2020
There are still strcmp() calls in the source, instead of memcmp(), so it's not going to work as-is... |
matthijskooijman commentedDec 31, 2020
Yeah, that's point 2. on my list above. But for some purposes (such as passing around buffers with embedded nuls) the PR is already useful as it stands (as long as you do not need comparison or some other operations). But as I said, ideally we'd fix at least the comparisons before merging this. |
cvwillegen commentedDec 31, 2020
I actually ran into this when doing an == on 2 NUL-containing strings :-) Indeed, I created#112 and you pointed me to this PR... |
aentinger left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hi@matthijskooijman 👋 and a happy new year 🎆
Thank you very much for all your good work 👍! I agree that both
- Adding some testcases with embedded nul bytes to verify that those actually work and stay working in the future (and increase coverage).
and
- Fixing String::equals (and related methods) with embedded nul characters (probably required to write proper testcases). This is also#112.
will be necessary for merging this PR. Maybe if you could provide test cases in the appropriate files forString::equals andString::compareTo? I don't see a need to provide embedded-nul-string test code for operators ==, !=, <, >, <=, >= as they are basically wrappers around the two functioned mentioned before.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
aentinger commentedJan 25, 2021
I assume a rebase is in order@matthijskooijman after my merging of#137? |
Instead of calling strlen in a dozen places, instead just callString::concat(s), which will then call strlen. This shrinks the codesize of these calls significantly, the StringAppendOperator example onthe Arduino Uno shrinks by 72 bytes. This change does incur a slightruntime cost, because there is one extra function call.
Previously, these methods took a nul-terminated string and appended itto the current buffer. The length argument (or length of the passedString object) was used to allocate memory, but was not used whenactually copying the string. This meant that: - If the passed length was too short, or the string passed in was not nul-terminated, the buffer would overflow. - If the string passed in contained embedded nul characters (i.e, among the first length characters), any characters after the embedded nul would not be copied (but len would be updated to indicate they were).In practice, neither of the above would occur, since the length passed isalways the known length of the string, usually as returned by strlen.However, to make this method public, and to allow using this concatmethod to pass in strings that are not nul-terminated, it should bechanged to be more robust.This commit changes the method to use memcpy instead of strcpy, copyingexactly the number of bytes passed in. For the current calls to thismethod, which pass a nul-terminated string, without embedded nulcharacters and a correct length, this behaviour should not change.However, this concat method can now also be used in the two casesmentioned above. Non-nul-terminated strings now work as expected and forstrings with embedded newlines the entire string is copied as-is,instead of leaving uninitialized memory after the embedded nul byte.Note that a lot of operations will still only see the string up to theembedded nul byte, but that's not really fixable unless we reimplementfunctions like strcmp.
Now concat(const char*, unsigned int) no longer requires anul-terminated string, we can simplify the concat(char) method to justpass the address of the single character instead of having to createbuffer with nul-termination.
This method is useful when receiving data from external sources thatpass an explicit length instead of a NUL-terminated string.
This constructor allows converting a buffer containing anon-nul-terminated string to a String object, by explicitely passing thelength.
Before, substring would (temporarily) add a \0 in the original string atthe end of the substring, so the substring could be copied into a newString object. However, now that the String::copy() method can work withnon-nul-terminated strings (by passing an explicit length), this trickis not needed and we can just call the copy method instead.
This just calls the char* version, but allows calling the method with auint8_t* as well (which is not uncommon for buffers).
This allows creating a String from a uint8_t[] or uint8_t* as well,without having to add explicit casts.
9a604e7 tod6fd84fComparematthijskooijman commentedJan 25, 2021
Yup, done. |
aentinger left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM 👍 Thank you@matthijskooijman 🚀
matthijskooijman commentedJan 25, 2021
Yay for merging this, but I didn't make the changes mentioned in#97 (review) yet... And there was still a fixup commit in here that should have been squashed before merging... Regardless, I believe that the changes in this PR are already useful and worth including, though support for strings with NUL is now not entirely complete and lacks testcases, so that still needs to be fixed (no time for that right now, though). |
cherry-pick from:arduino/ArduinoCore-API#97
When working with the Arduino String class, I've found that I couldn'tefficiently combine it with some external libraries that explicitelypass char* and length around, without nul-terminating their strings.This prompted me to modify and expose the concat (const char* cstr,unsigned int length) method, add a new String(const char* cstr, unsignedint length) constructor. While I was going over the string class, Ifound some other minor improvements, which are included here.This is a port ofarduino/Arduino#1936. The commits are identical,except for some improved commit messages and one commit was droppedsince that change was already made by someone else in the meantime.I've provided some testcases by updating the string examples:arduino/Arduino#9239If this is ok to merge, I'll also provide a PR for the referencedocumentation.cherry-pick from:arduino/ArduinoCore-API#97
When working with the Arduino String class, I've found that I couldn'tefficiently combine it with some external libraries that explicitelypass char* and length around, without nul-terminating their strings.This prompted me to modify and expose the concat (const char* cstr,unsigned int length) method, add a new String(const char* cstr, unsignedint length) constructor. While I was going over the string class, Ifound some other minor improvements, which are included here.This is a port ofarduino/Arduino#1936. The commits are identical,except for some improved commit messages and one commit was droppedsince that change was already made by someone else in the meantime.I've provided some testcases by updating the string examples:arduino/Arduino#9239If this is ok to merge, I'll also provide a PR for the referencedocumentation.cherry-pick from:arduino/ArduinoCore-API#97
codecov-commenter commentedOct 13, 2025
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## master #97 +/- ##==========================================- Coverage 96.42% 96.07% -0.36%========================================== Files 14 14 Lines 839 840 +1 ==========================================- Hits 809 807 -2- Misses 30 33 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When working with the Arduino String class, I've found that I couldn't efficiently combine it with some external libraries that explicitely pass char* and length around, without nul-terminating their strings. This prompted me to modify and expose the concat (const char* cstr, unsigned int length) method, add a new String(const char* cstr, unsigned int length) constructor. While I was going over the string class, I found some other minor improvements, which are included here.
This is a port ofarduino/Arduino#1936. The commits are identical, except for some improved commit messages and one commit was dropped since that change was already made by someone else in the meantime.
I've provided some testcases by updating the string examples:arduino/Arduino#9239
If this is ok to merge, I'll also provide a PR for the reference documentation.