Spooky C++

Posted by nablags 1 day ago

Comments

Comment by quuxplusone 1 day ago

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...

Comment by ranger_danger 1 day ago

but when using it as a parameter it IS stored as a variable, just only within that function, right?

Comment by quuxplusone 19 hours ago

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...

Comment by MathMonkeyMan 1 day ago

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.

    string_view remove_leading_spaces(string_view input) {
        auto new_start = find_if(input.begin(), input.end(), [](char ch) { return ch != ' '; });
        return string_view(new_start, input.end());
    }

The result of the undefined behavior in your program is surprising, though. As it usually is.

Comment by stefanos82 1 day ago

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`

Comment by dingaling911 1 day ago

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.

Comment by blastonico 1 day ago

  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

Comment by ranger_danger 1 day ago

> 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.

Comment by aw1621107 1 day ago

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.

Comment by dingaling911 1 day ago

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

Comment by ranger_danger 1 day ago

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 ' '.

Comment by dingaling911 1 day ago

string_view is literally a reference. Copying a string_view just copies the reference.

Comment by ranger_danger 1 day ago

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?

Comment by aw1621107 1 day ago

> 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.

Comment by dingaling911 19 hours ago

string_view is something like this

    struct string_view { const char * ref; }

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.

Comment by magackame 1 day ago

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.