[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I showed the bug mentioned in https://reviews.llvm.org/D49058 to a friend who didn't do much C++ recently, for a fresh look, and he provided a bunch of interesting feedback by explaining the way he didn't understand what the analyzer was trying to say.

1. When we call `c_str()`, the pointer is not dangling yet, not until the destructor or realloc is called. He didn't understand the report because he was trying to figure out why do we think the pointer is already dangling.
2. A generic "use after free" warning on the return site is confusing because the user would expect to see an actual "use" instead of just passing it around. We should be more specific, i.e. "Deallocated pointer returned to the caller".
3. We mention that there's a destructor, but the destructor is hard to see. Knowing the type of the destroyed object would help. Knowing that it's a temporary object would help.
4. The whole idea of "string has a buffer that would be destroyed when the string is destroyed and we shouldn't pass the pointer around" needs to be explained all together, rather than separated into different diagnostic pieces. The user needs to be somehow informed that this is how `std::string` operates because he doesn't necessarily know that.



================
Comment at: test/Analysis/dangling-internal-buffer.cpp:176
+  c = s.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+  s.clear();     // expected-note {{Method call is allowed to invalidate the internal buffer}}
+  consume(c);    // expected-warning {{Use of memory after it is freed}}
----------------
rnkovacs wrote:
> dcoughlin wrote:
> > What do you think about explicitly mentioning the name of the method here when we have it? This will make it more clear when there are multiple methods on the same line.
> > 
> > I also think that instead of saying "is allowed to" (which raises the question "by whom?") you could make it more direct.
> > 
> > How about:
> > "Inner pointer invalidated by call to 'clear'"
> > 
> > or, for the destructor "Inner pointer invalidated by call to destructor"?
> > 
> > What do you think?
> > 
> > If you're worried about this wording being to strong, you could weaken it with a "may be" to:
> > 
> > "Inner pointer may be invalidated by call to 'clear'"
> > 
> > 
> I like these, thanks! I went with the stronger versions now, as they seem to fit better with the warnings themselves.
> "Inner pointer invalidated by call to 'clear'"

I think the word "invalidated" may be confusing, how about "reallocated"? And "deallocated" in case of destructors.


https://reviews.llvm.org/D49360