Skip to content

String ::remove, ::trim have overlapping strncpy/memcpy, undefined results per standard specs #20

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
earlephilhower opened this issue Jul 18, 2018 · 3 comments
Closed

String ::remove, ::trim have overlapping strncpy/memcpy, undefined results per standard specs #20

earlephilhower opened this issue Jul 18, 2018 · 3 comments
Assignees
@facchinm
Labels
enhancement

Comments

@earlephilhower
Copy link

The ESP8266 Arduino port just discovered the following line in String::remove where strncpy is used with overlapping source and destination addresses (it's moving things backwards in the allocated buffer). See link for the valgrind warnings reported by our CI system.

ArduinoCore-avr/cores/arduino/WString.cpp

Lines 697 to 699 in b7c6076

char *writeTo = buffer + index;
len = len - count;
strncpy(writeTo, buffer + index + count,len - index);

str*cpy is undefined when you overlap source and destination

While it's true that on the AVR, an 8-bit MCU, str*cpy will be implemented in a byte-wise manner, this code has and will probably be copied to 16- and 32-bit systems where they may use optimizations which give strange results occasionally due to the overlap. A simple replacement of the strcpy with a memmove in this line clears this error and provides desired behavior on all systems.
@earlephilhower earlephilhower changed the title String class ::remove has overlapping strncpy, undefined results per standard specs String ::remove, ::trim have overlapping strncpy/memcpy, undefined results per standard specs Jul 18, 2018
@earlephilhower
Copy link
Author

Similar problem with the ::trim method:

ArduinoCore-avr/cores/arduino/WString.cpp

Lines 722 to 727 in b7c6076

char *begin = buffer;
while (isspace(*begin)) begin++;
char *end = buffer + len - 1;
while (isspace(*end) && end >= begin) end--;
len = end + 1 - begin;
if (begin > buffer) memcpy(buffer, begin, len);

Replacing memcpy with memmove on line 727 will fix this undefined usage on all systems.

@facchinm
Copy link
Member

@earlephilhower wow, thanks for sharing, we'll investigate if it applies to other 32bit cores and apply the fixes. Thanks a lot!

@facchinm facchinm self-assigned this Jul 19, 2018
facchinm added a commit to arduino/ArduinoCore-API that referenced this issue Oct 29, 2018
@facchinm
Fix undefined behaviour for overlapping String
b99609e 
Port of arduino/ArduinoCore-avr#20 , all credits goes to @earlephilhower
@facchinm
Copy link
Member

facchinm commented Dec 3, 2018

Applied in arduino/ArduinoCore-API@b99609e , now it's a matter of moving cores to API asap 🙂

@facchinm facchinm closed this as completed Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@facchinm facchinm

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@earlephilhower @facchinm