Skip to content

Fix String creation and concat issues found by Valgrind #4955

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

Merged
merged 2 commits into from
Jul 24, 2018
Merged

Fix String creation and concat issues found by Valgrind #4955

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e28d894
Fix String creation and concat issues
earlephilhower Jul 24, 2018
713faf8
Merge branch 'master' into stringfix1
earlephilhower Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions cores/esp8266/WString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ String::String(unsigned char value, unsigned char base) {
String::String(int value, unsigned char base) {
init();
char buf[2 + 8 * sizeof(int)];
itoa(value, buf, base);
if (base == 10) {
sprintf(buf, "%d", value);
} else {
itoa(value, buf, base);
}
*this = buf;
}

Expand All @@ -89,7 +93,11 @@ String::String(unsigned int value, unsigned char base) {
String::String(long value, unsigned char base) {
init();
char buf[2 + 8 * sizeof(long)];
ltoa(value, buf, base);
if (base==10) {
sprintf(buf, "%ld", value);
} else {
ltoa(value, buf, base);
}
*this = buf;
}

Expand Down Expand Up @@ -252,7 +260,22 @@ String & String::operator = (const __FlashStringHelper *pstr)
// /*********************************************/

unsigned char String::concat(const String &s) {
return concat(s.buffer, s.len);
// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if len includes the null term, then this is 1 byte too big. If not, then this is ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!s.buffer)
return 0;
if (s.len == 0)
return 1;
if (!reserve(newlen))
return 0;
memcpy(s.buffer + len, s.buffer, len);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How very inefficient... Every time I look at this implementation I want to migrate to a std::string-based one.
In that case, approving.

len = newlen;
return 1;
} else {
return concat(s.buffer, s.len);
}
}

unsigned char String::concat(const char *cstr, unsigned int length) {
Expand Down Expand Up @@ -283,13 +306,13 @@ unsigned char String::concat(char c) {

unsigned char String::concat(unsigned char num) {
char buf[1 + 3 * sizeof(unsigned char)];
itoa(num, buf, 10);
sprintf(buf, "%d", num);
return concat(buf, strlen(buf));
}

unsigned char String::concat(int num) {
char buf[2 + 3 * sizeof(int)];
itoa(num, buf, 10);
sprintf(buf, "%d", num);
return concat(buf, strlen(buf));
}

Expand All @@ -301,7 +324,7 @@ unsigned char String::concat(unsigned int num) {

unsigned char String::concat(long num) {
char buf[2 + 3 * sizeof(long)];
ltoa(num, buf, 10);
sprintf(buf, "%ld", num);
return concat(buf, strlen(buf));
}

Expand Down