This is why I call string_view a "parameter-only type": it's fine to take as a parameter, but not fine to return or to store as a variable. (For sane definitions of "fine" and "not fine," of course. The blog post demonstrates one way it can go "not fine.")
Yep; but a parameter is one of the special "safe" cases where the variable's lifetime is guaranteed to be shorter than the lifetime of its initializing expression. Other "safe" cases are catch-handler "parameters" (not that it makes much sense to catch a string_view) and for-loop "parameters" — i.e. `for (string_view sv : stuff())`. The most relevant of these cases by far is the case of plain old function parameters.
These are cases where the core language helps the programmer out, and basically makes a whole class of bugs — well, not impossible, but difficult to run into if you're not actually trying. There are other cases, e.g. `string_view sv = stuff();` as used in TFA, where the core language offers no help at all, but there's an easy solution available. ("Don't do that, then." Treat `string_view` as a "parameter-only type" and you'll never encounter that kind of problem.)
You returned a pointer to a temporary. It's nasty but easy to spot.
Would you return `input.c_str()`? You might think twice about that, but it's the same thing. My favorite fix is to take a `std::string_view` as the parameter.
std::string_view username = Sanitize("foobarbazboo");
^- temporary std::string created here
std::string_view Sanitize(const std::string& input) { ... }
^ ^- reference to that temporary string
+- returns a view to a temporary string
std::string_view username = // possibly data freed and dangling string
The other comments explained the same-length part.
As for the rest of the bug, my guess is:
1. Construct a temporary std::string for "foobarbazbug". The length of this string is 12 so it's stored on the stack due to the short/small string optimization.
2. Call Sanitize, passing a pointer to the temporary
3. Store the return value of Sanitize. Note that this return value is basically a pointer to the entire contents of the temporary, which due to SSO means the return value is actually a pointer to a specific bit of stack memory
4. Destroy the temporary.
5. Construct a std::string for the admin_id, reusing the stack slot for the temporary. This data also ends up on the stack due to SSO.
6. username == admin_id. username points to the stack memory formerly occupied by the argument passed to Sanitize(), which is now occupied by admin_id. As a result this comparison is effectively admin_id == admin_id, which is obviously true.
I suspect (but can't check for now) that due to the dependence on SSO lengths up to 16 should work in general, up to 22 (or 23? Can't remember) should work for libc++, and beyond that is not very likely to work at all.
But if Sanitize has a return type of string_view, then returning the temporary variable should make a copy of it (or move it), right? And the string "foobarbazboo" is static so that's not being freed either.
And the while loop shouldn't even run at all since front() never equals ' '.
But it's not actually taken as a reference (with &) anywhere I see... and even if it's just internally referencing the (static) string, there shouldn't be any issue with that memory either right?
> But it's not actually taken as a reference (with &) anywhere I see...
That is effectively what is happening internally. A string_view is morally a pointer/length pair; that pointer has to be initialized somehow.
> and even if it's just internally referencing the (static) string
It's not. The function call constructs a temporary std::string from the string literal, but once that is done the contents of the temporary are entirely independent from the string literal.
Where ref comes from the temporary string constructed via Sanitize("foobarbaz"). When that expression is done, the temporary string is destructed, leaving a dangling pointer.
This is why I call string_view a "parameter-only type": it's fine to take as a parameter, but not fine to return or to store as a variable. (For sane definitions of "fine" and "not fine," of course. The blog post demonstrates one way it can go "not fine.")
https://quuxplusone.github.io/blog/2018/03/27/string-view-is...
but when using it as a parameter it IS stored as a variable, just only within that function, right?
Yep; but a parameter is one of the special "safe" cases where the variable's lifetime is guaranteed to be shorter than the lifetime of its initializing expression. Other "safe" cases are catch-handler "parameters" (not that it makes much sense to catch a string_view) and for-loop "parameters" — i.e. `for (string_view sv : stuff())`. The most relevant of these cases by far is the case of plain old function parameters.
These are cases where the core language helps the programmer out, and basically makes a whole class of bugs — well, not impossible, but difficult to run into if you're not actually trying. There are other cases, e.g. `string_view sv = stuff();` as used in TFA, where the core language offers no help at all, but there's an easy solution available. ("Don't do that, then." Treat `string_view` as a "parameter-only type" and you'll never encounter that kind of problem.)
And then there's C++20 coroutine "parameters," where the core language pretty actively tries to trip you up. https://quuxplusone.github.io/blog/2019/07/10/ways-to-get-da...
You returned a pointer to a temporary. It's nasty but easy to spot.
Would you return `input.c_str()`? You might think twice about that, but it's the same thing. My favorite fix is to take a `std::string_view` as the parameter.
The result of the undefined behavior in your program is surprising, though. As it usually is.With GCC and `-fsanitize=address,undefined` flag(s) I have catched it during runtime.
I've got a nice `==31604==ERROR: AddressSanitizer: stack-use-after-scope on address`
Oh this is nasty. But the proposed solution of making admin_id a singleton is weird.
The real fix is don't pass in a temporary to the sanitize function.
> For example, this only works because the string foobarbazboo is the same length as admin_id
12 != 8
I don't get it...
> As you can guess, this is an example of undefined behavior causing unintuitive results.
No I couldn't... and what undefined behavior where? They don't explain anything.
The other comments explained the same-length part.
As for the rest of the bug, my guess is:
1. Construct a temporary std::string for "foobarbazbug". The length of this string is 12 so it's stored on the stack due to the short/small string optimization.
2. Call Sanitize, passing a pointer to the temporary
3. Store the return value of Sanitize. Note that this return value is basically a pointer to the entire contents of the temporary, which due to SSO means the return value is actually a pointer to a specific bit of stack memory
4. Destroy the temporary.
5. Construct a std::string for the admin_id, reusing the stack slot for the temporary. This data also ends up on the stack due to SSO.
6. username == admin_id. username points to the stack memory formerly occupied by the argument passed to Sanitize(), which is now occupied by admin_id. As a result this comparison is effectively admin_id == admin_id, which is obviously true.
I suspect (but can't check for now) that due to the dependence on SSO lengths up to 16 should work in general, up to 22 (or 23? Can't remember) should work for libc++, and beyond that is not very likely to work at all.
He means the string value of the variable admin_id, not the variable name admin_id.
And the bug comes from the temporary string object created when he passes in the string to Sanitize.
Sanitize("foobarbaz") -> creates temporary string -> string_view from temporary string -> returns string_view of temporary string
temporary string destructed
operations on returned string_view -> oops, use after free.q
But if Sanitize has a return type of string_view, then returning the temporary variable should make a copy of it (or move it), right? And the string "foobarbazboo" is static so that's not being freed either.
And the while loop shouldn't even run at all since front() never equals ' '.
string_view is literally a reference. Copying a string_view just copies the reference.
But it's not actually taken as a reference (with &) anywhere I see... and even if it's just internally referencing the (static) string, there shouldn't be any issue with that memory either right?
> But it's not actually taken as a reference (with &) anywhere I see...
That is effectively what is happening internally. A string_view is morally a pointer/length pair; that pointer has to be initialized somehow.
> and even if it's just internally referencing the (static) string
It's not. The function call constructs a temporary std::string from the string literal, but once that is done the contents of the temporary are entirely independent from the string literal.
string_view is something like this
Where ref comes from the temporary string constructed via Sanitize("foobarbaz"). When that expression is done, the temporary string is destructed, leaving a dangling pointer.The value of `admin_id` is "root_1231231", which is indeed the same length as "foobarbazboo". Otherwise, yeah, no idea where the bug comes from.