Everywhere: Remove AK::StringView(char const*) :^) #14555
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've made sure all tests pass and tested all applications on the Serenity menu. Hopefully I didn't break anything. |
Dexesttp
reviewed
Jul 11, 2022
Dexesttp
reviewed
Jul 11, 2022
Comment on lines
+262
to
+263
| auto const* error = strerror(key); | ||
| builder.append({ error, strlen(error) }); |
There was a problem hiding this comment.
Feels like an unfortunate side-effect of this change. Maybe it'd still be good to have a builder.append(char const*) API? Not sure, it could also be left as-is.
There was a problem hiding this comment.
The problem with this is that it introduces the same problem that this PR is trying to solve, but localizing it instead. I agree that it looks bad in many places, however that feels like the API that's forcing this should be fixed instead of introducing more char const* APIs.
|
Drafted in order to split up the changes. |
sin-ack
force-pushed
the
yeet-implicit-strlen
branch
from
61f1447
to
df18e54
Compare
sppmacd
added a commit
to essa-software/util
that referenced
this pull request
Jul 12, 2022
To make sure that strlen is not implicitly called at runtime (potentially overflowing a string and relying on the fact that it is null-terminated), UString constructors now take only string_view or array, so that the size is already known when calling the constructor. See SerenityOS/serenity#14555 for more details why const char* is bad here.
This makes the assumption that we never pass a stack-allocated char array to CheckedFormatString arguments (dbgln, outln, warnln). This assumption seems to hold true for the current state of Serenity code, at least. :^)
sin-ack
force-pushed
the
yeet-implicit-strlen
branch
from
df18e54
to
f3bf904
Compare
|
Re-drafted because I'm going amnesic. |
sin-ack
force-pushed
the
yeet-implicit-strlen
branch
5 times, most recently
from
6563114
to
54094f6
Compare
This moves out the calculation of the char* out to the formatter. Additionally, we now print (null) when a null pointer is passed.
Since all uses of SourceGenerator are with literal strings, there is no need to burden generators with the sv suffix.
These are mostly minor mistakes I've encountered while working on the
removal of StringView(char const*). The usage of builder.put_string over
Format<FormatString>::format is preferrable as it will avoid the
indirection altogether when there's no formatting to be done. Similarly,
there is no need to do format(builder, "{}", number) when
builder.put_u64(number) works equally well.
Additionally a few Strings where only constant strings were used are
replaced with StringViews.
It didn't feel right to add sv suffixes to 2-character strings, so I added this convenience constructor.
This removes the need for calculating each string's length during ErrorType use at the cost of storing it within the binary.
Previously it would rely on the implicit StringView conversions. Now the decode_equal function will directly use StringViews.
StringView was used where possible. Some utilities still use libc functions which expect null-terminated strings, so String objects were used there instead.
These were accidental (or leftover) uses of String::characters() to construct StringViews through its StringView(char const*) constructor. Since this constructor is due to be removed, this will no longer work. Plus this prevents strlen from being run on these strings unnecessarily.
These convenience templates allow the following to be written as before:
TRY(Core::System::pledge("promises..."));
TRY(Core::System::pledge("promises...", "execpromises..."));
TRY(Core::System::unveil("path", "permissions"));
TRY(Core::System::unveil(nullptr, nullptr));
Other uses must now append sv to any literal string passed to pledge and
unveil.
This commit moves the length calculations out to be directly on the StringView users. This is an important step towards the goal of removing StringView(char const*), as it moves the responsibility of calculating the size of the string to the user of the StringView (which will prevent naive uses causing OOB access).
Error::from_string_literal now takes direct char const*s, while Error::from_string_view does what Error::from_string_literal used to do: taking StringViews. This change will remove the need to insert `sv` after error strings when returning string literal errors once StringView(char const*) is removed. No functional changes.
Each of these strings would previously rely on StringView's char const* constructor overload, which would call __builtin_strlen on the string. Since we now have operator ""sv, we can replace these with much simpler versions. This opens the door to being able to remove StringView(char const*). No functional changes.
This prevents us from needing a sv suffix, and potentially reduces the need to run generic code for a single character (as contains, starts_with, ends_with etc. for a char will be just a length and equality check). No functional changes.
While null StringViews are just as bad, these prevent the removal of StringView(char const*) as that constructor accepts a nullptr. No functional changes.
This prevents a bunch of utilities from using StringView for their arguments.
This allowed passing in a nullptr for the StringView which will not be possible once StringView(char const*) is removed.
During the removal of StringView(char const*), all users of these functions were removed, and they are of dubious value (relying on implicit StringView conversion).
The StringView(char const*) constructor is being removed, and there was only a few users of this left, which are also cleaned up in this commit.
We now explicitly disallow this.
This constructor relied on running strlen implicitly on its argument, thereby potentially causing out-of-bound reads (some of which were caught a few days ago). The removal of this constructor ensures that the caller must explicitly pass the size of the string by either: 1) Using operator""sv on literal strings; or 2) Calling strlen explicitly, making it clear that the size of the view is being calculated at runtime.
Dexesttp
approved these changes
Jul 12, 2022
There was a problem hiding this comment.
Alright, I re-read through the whole thing (well, I read most commits, skimmed through "Everywhere: Explicitly specify the size in StringView constructors", "Everywhere: Split Error::from_string_literal and Error::from_string_view" and "Everywhere: Replace single-char StringView op. arguments with chars", and trusting you on "Everywhere: Add sv suffix to strings relying on StringView(char const*)" cuz I'm not reading that one in detail :^) )
So, looks fine to me!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes at least one potential source of safety errors by
preventing the passing of arbitrary character pointers to the StringView
constructor, which would then call strlen on it implicitly and
potentially read out-of-bounds. This is not really preventable without
completely disallowing the use of this style. Users of StringView should
prefer to use the sv suffix for constant strings whereever possible.
It turns out we used this constructor all over the codebase, so this is
a very chonky commit.
No functional changes, as this just marks constant strings as constant
and moves the strlen out to the StringView use site when the string
isn't constant.