Skip to content

use memcpy instead of strcpy in WString.cpp #6027

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Apr 30, 2019

I can happen String carries binary data, this simple update allows it.
Also, memcpy is more efficient than strcpy when size is known.
It's an harmless temporary fix until Webserver's painful arg("plain") can be worked out.
@LaborEtArs

I can happen String carries binary data, this simple update allows it.
Also, memcpy is more efficient than strcpy when size is known.
It's an harmless temporary fix until Webserver's painful arg("plain") can be worked out.
@d-a-v d-a-v requested review from earlephilhower and devyte April 30, 2019 13:41
@earlephilhower
Copy link
Collaborator

Strings must not contain 0s, that was one of the assumptions of the SSO optimization (and I believe other bits of the original String class).

SSO uses strlen to get the length of a string in SSO (allows 1 more byte in the String object for text), so this won't work as-is.

It would be possible to rewrite SSO using the byte (sizeof(String)-1) as length, and adjusting the maximum SSO string size by 1.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Fixes needed per @earlephilhower for SSO case

@earlephilhower
Copy link
Collaborator

Also, random thought: memcpy may be unsafe, would suggest using memmove unless we can verify there is no potential for overlapping src/dest. Had other problems in String because of stuff like this.

@earlephilhower earlephilhower added this to the 2.6.0 milestone May 19, 2019
@KlaasjanN
Copy link

I may have encountered some issues where the string is empty. In the past this did not seem to cause any issues, but it seems that the switch to memmove is causing problems. Haven't had time to check the underlying code and to do full analysis, but can you please include testing the String length, before starting any memmove operation?

An example: I just figured out that on 2.5.2 the replace function is not behaving as it was in the past. That is: it leads to a crash, and I suspect that it is due to the string being empty (or perhaps when there is nothing to replace in a valid string?). With the 'old' replace function this didn't happen.

I'm sorry that I'm not able to provide any further details or sample code right now, but just wanted to contribute my two cents.

@earlephilhower
Copy link
Collaborator

This patch isn't applied yet, and memmove handles all cases memcpy would, just ensuring data integrity.

If you can find a case that crashes that shouldn't in the current release, @KlaasjanN, we would really appreciate an issue on it. My own quick testing doesn't show any problems with empty strings:

void setup() {
  Serial.begin(115200);
  String a("this is a test");
  a.replace("is", emptyString);
  Serial.printf("'%s'\n", a.c_str());
  a.replace(" ", "");
  Serial.printf("'%s'\n", a.c_str());
  String b;
  b.replace("abcd", "");
  Serial.printf("'%s'\n", b.c_str());
  b.replace("", "");
  Serial.printf("'%s'\n", b.c_str());
}

void loop() {
}

Gives

'th  a test'
'thatest'
''
''

and no crashes.

earlephilhower added a commit to earlephilhower/Arduino that referenced this pull request May 27, 2019
Supercedes esp8266#6027

Make SSO more generic by keeping track of its length explicitly,
allowing for embedded \0s to exist in the String (just like the non-SSO
ones).
@d-a-v d-a-v closed this May 28, 2019
earlephilhower added a commit that referenced this pull request Jun 5, 2019
Supercedes #6027

Make SSO more generic by keeping track of its length explicitly,
allowing for embedded \0s to exist in the String (just like the non-SSO
ones).

Use memmove/memcpy_P when we know the length of a string to save CPU
time.

Add tests to inject \0s in a String to ensure it is still working as
designed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants