Skip to content

Avoid writing to const storage#8463

Merged
mcspr merged 1 commit intoesp8266:masterfrom
paulocsanz:master
Jan 28, 2022
Merged

Avoid writing to const storage#8463
mcspr merged 1 commit intoesp8266:masterfrom
paulocsanz:master

Conversation

@paulocsanz
Copy link
Contributor

@paulocsanz paulocsanz commented Jan 25, 2022

Fixes #8462

This avoids the null termination requirement of both String::substring and String::lastIndexOf by using APIs that don't require it. So we can stop writing to the buffer inside of const functions.

There might be a off by one error there, I need to take a better look at the code and would love if someone could review.

@earlephilhower
Copy link
Collaborator

earlephilhower commented Jan 25, 2022

Seems to have broken some host tests:

-------------------------------------------------------------------------------
String SSO works
-------------------------------------------------------------------------------
core/test_string.cpp:322
...............................................................................

core/test_string.cpp:420: FAILED:
  REQUIRE( s.length() == 5 )
with expansion:
  15 == 5

@earlephilhower
Copy link
Collaborator

earlephilhower commented Jan 25, 2022

Arduino/tests/host/core/test_string.cpp

Lines 417 to 420 in 30b0450

s = "0123456789abcde";
s = s.substring(s.indexOf('a'));
REQUIRE(s == "abcde");
REQUIRE(s.length() == 5);

@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jan 25, 2022

Will fix it, thanks! How can I run the host tests locally so I don't clutter the project's CI and can iterate over the code faster?

@earlephilhower
Copy link
Collaborator

earlephilhower commented Jan 25, 2022

Will fix it, thanks! How can I run the host tests locally so I don't clutter the project's CI and can iterate over the code faster?

In tests/host just run make test. You'll probably need valgrind and a couple other dependencies which will show up.

@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jan 25, 2022

Hmm, I'm getting a bunch of compilation errors because of std::enable_if_t, std::is_same_v, std::decay_t and #include "../lib/littlefs/lfs.h, but it seems I fixed the test.

@mcspr
Copy link
Collaborator

mcspr commented Jan 25, 2022

Were you intending to fix wbuffer() as well? It seems the const_cast originates b/c of the const qualifier(s), where the compiler did warn about the const misnomer but it was circumvented

mcspr
mcspr reviewed Jan 25, 2022
View reviewed changes
@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jan 25, 2022 •
edited
Loading

Yeah, I would prefer to make wbuffer into non const, should I change it in this PR too then?

I will mess with this PR's code after work today.

@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jan 26, 2022 •
edited
Loading

@d-a-v Done, your trick worked!

I also changed wbuffer to make it non const.

d-a-v
d-a-v approved these changes Jan 26, 2022
View reviewed changes
Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

LGTM

mcspr
mcspr reviewed Jan 26, 2022
View reviewed changes
@mcspr mcspr merged commit 9f536e6 into esp8266:master Jan 28, 2022
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
@hasenradball
WString: avoid writing to const storage (esp8266#8463)
60ac463 
This avoids the null termination requirement of both String::substring and String::lastIndexOf by using APIs that don't require it. So we can stop writing to the buffer inside of const functions.

I also changed wbuffer to make it non const.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcspr mcspr mcspr approved these changes

@d-a-v d-a-v d-a-v approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Avoid writing in const functions, as it can technically be UB

4 participants

@paulocsanz @earlephilhower @mcspr @d-a-v