Skip to content
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

Introduce std::string_view into wider use #14368

Merged
merged 17 commits into from Feb 17, 2024
Merged

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Feb 13, 2024

lots of general conversion and many low hanging fruits

To do

This PR is Ready for Review.

How to test

  1. check that nothing is broken (somehow)

@sfan5 sfan5 added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Performance Code quality labels Feb 13, 2024
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Code looks good. Haven't smoke-tested yet.

src/mapblock.cpp Show resolved Hide resolved
src/util/string.h Show resolved Hide resolved
src/script/common/helper.cpp Show resolved Hide resolved
Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

For the future, it would be nice if we could handle the use of std::string_view like const correctness:

void fun(const T *); // if this is good enough
void fun(T *); // then don't use this

void fun(std::string_view); // if this is good enough
void fun(const std::string &); // then don't use this

src/util/string.h Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Show resolved Hide resolved
src/util/string.h Outdated Show resolved Hide resolved
@@ -313,14 +324,34 @@ inline std::string trim(const std::string &str)
return str.substr(front, back - front);
}

// If input was a temporary string keep it one to make sure patterns like
// trim(func_that_returns_str()) are predictable regarding memory allocation
// and don't lead to UAF. ↓ ↓ ↓
Copy link
Member

Choose a reason for hiding this comment

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

It only leads to UaFs if the string view is accessed outside of the full expression, right?
So std::string a = std::string(trim(func_that_returns_str()));, for example, should be fine anyways.

It's a bit weird to have two trim with different signatures. Could rename them to trim_v and trim_s.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that should be fine yes.
The problem would exist if you do auto a = trim(func_that_returns_str());. The string goes out of scope at the end of the line.

Generally I did this so I don't have to touch the many usages of trim that work like this.
I can add a // TODO: this function should be removed or renamed to trim_s to make the distinction clear. if you think that's a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

I can add a // TODO: this function should be removed or renamed to trim_s to make the distinction clear. if you think that's a good idea.

Idk. I guess the overloaded trim is fine for now. ¯\_(ツ)_/¯

src/util/string.h Outdated Show resolved Hide resolved
src/util/string.h Outdated Show resolved Hide resolved
@sfan5
Copy link
Member Author

sfan5 commented Feb 16, 2024

For the future, it would be nice if we could handle the use of std::string_view like const correctness:

Definitely. Feel free to write this down in the guidelines.

doc/lua_api.md Show resolved Hide resolved
src/util/sha1.cpp Outdated Show resolved Hide resolved
@Desour
Copy link
Member

Desour commented Feb 16, 2024

Tested shortly with asan, ubsan and leaksan. Found no issues.

Definitely. Feel free to write this down in the guidelines.

Done.

@Desour Desour added One approval ✅ ◻️ @ Engine Core What happens inside the very engine labels Feb 16, 2024
src/database/database.h Outdated Show resolved Hide resolved
src/script/lua_api/l_item.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_item.cpp Outdated Show resolved Hide resolved
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@appgurueu appgurueu merged commit 6ca214f into minetest:master Feb 17, 2024
15 checks passed
@sfan5 sfan5 deleted the sview branch February 17, 2024 14:36
appgurueu pushed a commit to y5nw/minetest that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality @ Engine Core What happens inside the very engine Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Performance >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants