std::string_view encourages use-after-free; the Core Guidelines Checker doesn't complain

[kcc]

[please excuse me if this has been discussed before]

#include <iostream>
#include <string>
#include <string_view>

int main() {
  std::string s = "Hellooooooooooooooo ";
  std::string_view sv = s + "World\n";
  std::cout << sv;
}

Here we have a heap-use-after-free bug which is easy to spot if you know what to look for, but the Core Guidelines Checker in VS++17 is silent (confirmed by @AndrewPardoe). This might be something missing in the checker, but I suspect that this is actually missing in the guidelines themselves. Moreover, I don't see how we can reject code like this w/o rejecting lots of other safe C++17 code.

Thoughts?

See also: https://bugs.llvm.org/show_bug.cgi?id=34729

[jyasskin]

It's been discussed. The obvious solution of deleting string_view(string&&) breaks the primary use of string_view as a function argument.

@kcc, you primarily work in an enormous codebase that calls this type StringPiece. Do you feel like its existence is a good tradeoff there?

[jwakely]

See also: https://bugs.llvm.org/show_bug.cgi?id=34729

This is a feature, not a bug.

However, I've just skimmed the guidelines and don't see anything specifically about avoiding dangling views. It should be advised against, even if it's made possible by the API and can't be enforced by the checkers.

[MikeGitb]

Supposedly, the lifetime profile should catch any cases of dangling pointers (wasn't that the claim at cppcon 15?). So msvc should be able to catch them, but I don't remember if that function ever left the beta stage.

[kcc]

calls this type StringPiece

It's been renamed to absl::string_view. We've being finding many misuses of this class (causing heap-use-after-free and stack-use-after-return) since 2011.
My POV is heavily biased due to countless hours we've spent debugging those, even with ASAN.
IMHO string_view is a disaster.
Of course, removing this class does not solve the problem of the language and I was hoping that at least the core guidelines would forbid this type of code (not just string_view).

can't be enforced by the checkers.

If a guideline can't be enforced, does it still have any value?

[jyasskin]

@kcc Have you done anything to get it removed from the codebases you've found the misuses in? What blocked you? Remember, I asked if it was a good tradeoff, not if it had no issues.

[kcc]

I haven't -- it's used everywhere, and when used carefully it brings lots of performance benefits.
So, let me correct myself: it's a stability/security disaster and a performance win.
tradeoffs are hard here.

[kcc]

And again, my complaint is not so much about std::string_view, but more about the core guidelines checker not reporting this and similar code as a problem (non-core).

[JonasToth]

Out of interest: could we find patterns of the misuse? Clang-tidy could catch the string_view sv = s + "string";. So even if we can't find all for now, we could find some, maybe even most cases.

[tamaskenez]

In the range-v3 library it is explicitly disallowed to create a range (which is a view) from an rvalue container. Wondering if something similar could be done for string_view.
https://ericniebler.github.io/std/wg21/D4128.html#ranges-cannot-own-elements

[MikeGitb]

@tamaskenez: As @jyasskin said, that would eliminate a few useful use cases when it is used as a function parameter. You also can't enforce this in string_view's api itself, as the conversion string-> view is part of std::string's interface.

I'm not saying that it might not be worth to make this trade-off, but Imho this is not a guideline/interface issue, but a tool issue. For all intents and purposes std::string_view should be treated like a glorified raw pointer or iterator (except when used as a parameter).
And imho a proper static analysis tool (including the compiler) should be able to see that I'm creating a pointer to a temporary here.

[kcc]

a proper static analysis tool

Are you aware of any such tool?
(and to make things more realistic, it should catch a general case, not just the patter where the literal std::string_view is used.

[villevoutilainen]

See https://bugs.llvm.org/show_bug.cgi?id=34500

The problematic cases should be caught by static analyzers, but are not, because the tools are buggy.

[neilmacintosh]

@kcc so if I understand correctly, you are just complaining about tool implementation, rather than the guideline itself? Why would you not raise an issue directly with your tool vendor?

[MikeGitb]

@kcc: Sorry, I expressed myself poorly. What I meant was
std::string_view should be treated like a glorified pointer (potentially by special logic) and IF that is the case, then any tool should be able to detect that you are creating a std::string_view (aka pointer) to a temporary (not saying any tool is actually doing it).

Apparently this is not where we are, but it wouldn't require black magic to do so. This is why I call it a tool issue.

Now, my understanding of the demos @neilmacintosh gave at cppcon 2015 during Herb's talk (Neil, please correct me if I'm wrong) was that the goal for Microsoft's analyser was to do even better and actually detect that string_view has an internal pointer, that points to data, whose lifetime is coupled to that of the temporary string. Obviously, they didn't demo string_view back then, but std::vector and iterators, which should be a very similar problem.
Afaik they did demo more a proof of concept instead of a released product back then, but it shows that my expectations that your case should be detectable by a tool are not unreasonable.

Btw.: Of course that there also has to be a solution for custom types, but actually, I would be very happy, if tools knew about std::string_view and gsl::span and checked my code against their specification and not just a particular implementation thereof.

[kcc]

Making tools do something special for std::string_view or other hand-picked types is counter-productive, imho. Users will start to rely on checker to find misuses of std::string_view and won't realize that a similar situation in their classes is not reported.

Not sure if we want to discuss this here, but there is also another related problem in the current checkers: they work on a single translation unit (right?). I.e. if the method that leaks internal state is declared in another TU, the checker is blind.