[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
This revision was automatically updated to reflect the committed changes.
Closed by commit rC337463: [analyzer] Add support for more basic_string API in
(authored by rkovacs, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D49360
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -32,8 +32,8 @@
// This is a trick to gain access to PtrSet's Factory.
namespace clang {
namespace ento {
-template<> struct ProgramStateTrait
- : public ProgramStatePartialTrait {
+template <>
+struct ProgramStateTrait : public ProgramStatePartialTrait {
static void *GDMIndex() {
static int Index = 0;
return
@@ -46,7 +46,10 @@
class DanglingInternalBufferChecker
: public Checker {
- CallDescription CStrFn, DataFn;
+
+ CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
+ InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
+ ShrinkToFitFn, SwapFn;
public:
class DanglingBufferBRVisitor : public BugReporterVisitor {
@@ -81,7 +84,17 @@
}
};
- DanglingInternalBufferChecker() : CStrFn("c_str"), DataFn("data") {}
+ DanglingInternalBufferChecker()
+ : AppendFn("append"), AssignFn("assign"), ClearFn("clear"),
+CStrFn("c_str"), DataFn("data"), EraseFn("erase"), InsertFn("insert"),
+PopBackFn("pop_back"), PushBackFn("push_back"), ReplaceFn("replace"),
+ReserveFn("reserve"), ResizeFn("resize"),
+ShrinkToFitFn("shrink_to_fit"), SwapFn("swap") {}
+
+ /// Check whether the function called on the container object is a
+ /// member function that potentially invalidates pointers referring
+ /// to the objects's internal buffer.
+ bool mayInvalidateBuffer(const CallEvent ) const;
/// Record the connection between the symbol returned by c_str() and the
/// corresponding string object region in the ProgramState. Mark the symbol
@@ -94,6 +107,37 @@
} // end anonymous namespace
+// [string.require]
+//
+// "References, pointers, and iterators referring to the elements of a
+// basic_string sequence may be invalidated by the following uses of that
+// basic_string object:
+//
+// -- TODO: As an argument to any standard library function taking a reference
+// to non-const basic_string as an argument. For example, as an argument to
+// non-member functions swap(), operator>>(), and getline(), or as an argument
+// to basic_string::swap().
+//
+// -- Calling non-const member functions, except operator[], at, front, back,
+// begin, rbegin, end, and rend."
+//
+bool DanglingInternalBufferChecker::mayInvalidateBuffer(
+const CallEvent ) const {
+ if (const auto *MemOpCall = dyn_cast()) {
+OverloadedOperatorKind Opc = MemOpCall->getOriginExpr()->getOperator();
+if (Opc == OO_Equal || Opc == OO_PlusEqual)
+ return true;
+return false;
+ }
+ return (isa(Call) || Call.isCalled(AppendFn) ||
+ Call.isCalled(AssignFn) || Call.isCalled(ClearFn) ||
+ Call.isCalled(EraseFn) || Call.isCalled(InsertFn) ||
+ Call.isCalled(PopBackFn) || Call.isCalled(PushBackFn) ||
+ Call.isCalled(ReplaceFn) || Call.isCalled(ReserveFn) ||
+ Call.isCalled(ResizeFn) || Call.isCalled(ShrinkToFitFn) ||
+ Call.isCalled(SwapFn));
+}
+
void DanglingInternalBufferChecker::checkPostCall(const CallEvent ,
CheckerContext ) const {
const auto *ICall = dyn_cast();
@@ -127,7 +171,7 @@
return;
}
- if (isa(ICall)) {
+ if (mayInvalidateBuffer(Call)) {
if (const PtrSet *PS = State->get(ObjRegion)) {
// Mark all pointer symbols associated with the deleted object released.
const Expr *Origin = Call.getOriginExpr();
@@ -161,8 +205,8 @@
CleanedUpSet = F.remove(CleanedUpSet, Symbol);
}
State = CleanedUpSet.isEmpty()
- ? State->remove(Entry.first)
- : State->set(Entry.first, CleanedUpSet);
+ ? State->remove(Entry.first)
+ : State->set(Entry.first, CleanedUpSet);
}
}
C.addTransition(State);
@@ -183,7 +227,7 @@
SmallString<256> Buf;
llvm::raw_svector_ostream OS(Buf);
- OS << "Pointer to dangling buffer was obtained here";
+ OS << "Dangling inner pointer obtained here";
PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
N->getLocationContext());
return std::make_shared(Pos, OS.str(), true,
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
NoQ added inline comments.
Comment at: test/Analysis/dangling-internal-buffer.cpp:175
std::string s;
- {
-c = s.c_str();
- }
- consume(c); // no-warning
+ 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}}
rnkovacs wrote:
> dcoughlin wrote:
> > In other parts of clang we use the term "inner pointer" to mean a pointer
> > that will be invalidated if its containing object is destroyed
> > https://clang.llvm.org/docs/AutomaticReferenceCounting.html#interior-pointers.
> > There are existing attributes that use this term to specify that a method
> > returns an inner pointer.
> >
> > I think it would be good to use the same terminology here. So the
> > diagnostic could be something like "Dangling inner pointer obtained here".
> I feel like I should also retitle the checker to `DanglingInnerBuffer` to
> remain consistent. What do you think?
My intuition suggests that we should remove the word "Dangling" from the
checker name, because our checker names are usually indicating what they check,
not what bugs they find. Eg., `MallocChecker` doesn't find all mallocs, it
checks that mallocs are used correctly. This checker checks that pointers to
inner buffers are used correctly, so we may call it `InnerPointerChecker` or
something like that.
https://reviews.llvm.org/D49360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
NoQ accepted this revision. NoQ added a comment. Let's commit this patch and make another round later, as we gather ideas for better names and messages. https://reviews.llvm.org/D49360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
rnkovacs updated this revision to Diff 155944.
rnkovacs marked 2 inline comments as done.
rnkovacs added a reviewer: dcoughlin.
rnkovacs added a comment.
Note messages updated.
https://reviews.llvm.org/D49360
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -2,13 +2,35 @@
namespace std {
-template< typename CharT >
+typedef int size_type;
+
+template
class basic_string {
public:
+ basic_string();
+ basic_string(const CharT *s);
+
~basic_string();
+ void clear();
+
+ basic_string =(const basic_string );
+ basic_string +=(const basic_string );
+
const CharT *c_str() const;
const CharT *data() const;
CharT *data();
+
+ basic_string (size_type count, CharT ch);
+ basic_string (size_type count, CharT ch);
+ basic_string (size_type index, size_type count);
+ basic_string (size_type index, size_type count, CharT ch);
+ basic_string (size_type pos, size_type count, const basic_string );
+ void pop_back();
+ void push_back(CharT ch);
+ void reserve(size_type new_cap);
+ void resize(size_type count);
+ void shrink_to_fit();
+ void swap(basic_string );
};
typedef basic_string string;
@@ -23,73 +45,70 @@
void consume(const char16_t *) {}
void consume(const char32_t *) {}
-void deref_after_scope_char_cstr() {
- const char *c;
+void deref_after_scope_char(bool cond) {
+ const char *c, *d;
{
std::string s;
-c = s.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
+c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+d = s.data(); // expected-note {{Dangling inner pointer obtained here}}
+ }// expected-note {{Inner pointer invalidated by call to destructor}}
+ // expected-note@-1 {{Inner pointer invalidated by call to destructor}}
std::string s;
const char *c2 = s.c_str();
- consume(c); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
-}
-
-void deref_after_scope_char_data() {
- const char *c;
- {
-std::string s;
-c = s.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::string s;
- const char *c2 = s.data();
- consume(c); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
+ if (cond) {
+// expected-note@-1 {{Assuming 'cond' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'cond' is 0}}
+// expected-note@-4 {{Taking false branch}}
+consume(c); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ }
}
void deref_after_scope_char_data_non_const() {
char *c;
{
std::string s;
-c = s.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
+c = s.data(); // expected-note {{Dangling inner pointer obtained here}}
+ } // expected-note {{Inner pointer invalidated by call to destructor}}
std::string s;
char *c2 = s.data();
consume(c); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
}
-
-void deref_after_scope_wchar_t_cstr() {
- const wchar_t *w;
+void deref_after_scope_wchar_t(bool cond) {
+ const wchar_t *c, *d;
{
-std::wstring ws;
-w = ws.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::wstring ws;
- const wchar_t *w2 = ws.c_str();
- consume(w); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
-}
-
-void deref_after_scope_wchar_t_data() {
- const wchar_t *w;
- {
-std::wstring ws;
-w = ws.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::wstring ws;
- const wchar_t *w2 = ws.data();
- consume(w); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
+std::wstring s;
+c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
rnkovacs added inline comments.
Comment at: test/Analysis/dangling-internal-buffer.cpp:175
std::string s;
- {
-c = s.c_str();
- }
- consume(c); // no-warning
+ 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}}
dcoughlin wrote:
> In other parts of clang we use the term "inner pointer" to mean a pointer
> that will be invalidated if its containing object is destroyed
> https://clang.llvm.org/docs/AutomaticReferenceCounting.html#interior-pointers.
> There are existing attributes that use this term to specify that a method
> returns an inner pointer.
>
> I think it would be good to use the same terminology here. So the diagnostic
> could be something like "Dangling inner pointer obtained here".
I feel like I should also retitle the checker to `DanglingInnerBuffer` to
remain consistent. What do you think?
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}}
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.
https://reviews.llvm.org/D49360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
dcoughlin added a comment.
It is really nice to see this checker take shape! Some drive by diagnostic
comments in line.
Comment at: test/Analysis/dangling-internal-buffer.cpp:175
std::string s;
- {
-c = s.c_str();
- }
- consume(c); // no-warning
+ 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}}
In other parts of clang we use the term "inner pointer" to mean a pointer that
will be invalidated if its containing object is destroyed
https://clang.llvm.org/docs/AutomaticReferenceCounting.html#interior-pointers.
There are existing attributes that use this term to specify that a method
returns an inner pointer.
I think it would be good to use the same terminology here. So the diagnostic
could be something like "Dangling inner pointer obtained here".
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}}
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'"
https://reviews.llvm.org/D49360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
NoQ added a comment. Hmm, the destructor-specific message was pretty good, can we keep it? It should be possible to print a different message depending on the program point within `N`. https://reviews.llvm.org/D49360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
rnkovacs added a comment. In https://reviews.llvm.org/D49360#1163113, @NoQ wrote: > Also we rarely commit to adding a test for every single supported API > function; bonus points for that, but usually 2-3 functions from a series of > similar functions is enough :) Um, okay, noted for next time :) https://reviews.llvm.org/D49360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
rnkovacs updated this revision to Diff 155770.
rnkovacs marked an inline comment as done.
rnkovacs edited the summary of this revision.
rnkovacs added a comment.
Added standard quote, marking the section about non-member functions that may
also invalidate the buffer as a TODO.
Also changed the note message to that suggested by @NoQ (thanks!). All tests
pass now.
https://reviews.llvm.org/D49360
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -2,13 +2,35 @@
namespace std {
-template< typename CharT >
+typedef int size_type;
+
+template
class basic_string {
public:
+ basic_string();
+ basic_string(const CharT *s);
+
~basic_string();
+ void clear();
+
+ basic_string =(const basic_string );
+ basic_string +=(const basic_string );
+
const CharT *c_str() const;
const CharT *data() const;
CharT *data();
+
+ basic_string (size_type count, CharT ch);
+ basic_string (size_type count, CharT ch);
+ basic_string (size_type index, size_type count);
+ basic_string (size_type index, size_type count, CharT ch);
+ basic_string (size_type pos, size_type count, const basic_string );
+ void pop_back();
+ void push_back(CharT ch);
+ void reserve(size_type new_cap);
+ void resize(size_type count);
+ void shrink_to_fit();
+ void swap(basic_string );
};
typedef basic_string string;
@@ -23,73 +45,70 @@
void consume(const char16_t *) {}
void consume(const char32_t *) {}
-void deref_after_scope_char_cstr() {
- const char *c;
+void deref_after_scope_char(bool cond) {
+ const char *c, *d;
{
std::string s;
c = s.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
+d = s.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+ }// expected-note {{Method call is allowed to invalidate the internal buffer}}
+ // expected-note@-1 {{Method call is allowed to invalidate the internal buffer}}
std::string s;
const char *c2 = s.c_str();
- consume(c); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
-}
-
-void deref_after_scope_char_data() {
- const char *c;
- {
-std::string s;
-c = s.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::string s;
- const char *c2 = s.data();
- consume(c); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
+ if (cond) {
+// expected-note@-1 {{Assuming 'cond' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'cond' is 0}}
+// expected-note@-4 {{Taking false branch}}
+consume(c); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ }
}
void deref_after_scope_char_data_non_const() {
char *c;
{
std::string s;
c = s.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
+ } // expected-note {{Method call is allowed to invalidate the internal buffer}}
std::string s;
char *c2 = s.data();
consume(c); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
}
-
-void deref_after_scope_wchar_t_cstr() {
- const wchar_t *w;
+void deref_after_scope_wchar_t(bool cond) {
+ const wchar_t *c, *d;
{
-std::wstring ws;
-w = ws.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::wstring ws;
- const wchar_t *w2 = ws.c_str();
- consume(w); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
-}
-
-void deref_after_scope_wchar_t_data() {
- const wchar_t *w;
- {
-std::wstring ws;
-w = ws.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::wstring ws;
- const wchar_t *w2 = ws.data();
- consume(w); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
+std::wstring s;
+c =
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Cool! I don't have a strong preference with respect to whitelist vs. blacklist;
your approach is safer but listing functions that don't immediately invalidate
the buffer would allow us to avoid hard-to-detect false negatives while
pretending that our users would notice and report easy-to-fix false positives
for us. Also we rarely commit to adding a test for every single supported API
function; bonus points for that, but usually 2-3 functions from a series of
similar functions is enough :)
Comment at:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:112-124
+ if (const auto *MemOpCall = dyn_cast()) {
+OverloadedOperatorKind Opc = MemOpCall->getOriginExpr()->getOperator();
+if (Opc == OO_Equal || Opc == OO_PlusEqual)
+ return true;
+return false;
+ }
+ return (isa(Call) || Call.isCalled(AppendFn) ||
That quote from the Standard would look great here.
Repository:
rC Clang
https://reviews.llvm.org/D49360
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker
rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet,
baloghadamsoftware, whisperity.
A pointer referring to the elements of a `basic_string` may be invalidated by
calling a non-const member function, except `operator[]`, `at`, `front`,
`back`, `begin`, `rbegin`, `end`, and `rend`. The checker now warns if the
pointer is used after such operations.
FIXME: warning messages.
Repository:
rC Clang
https://reviews.llvm.org/D49360
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -2,13 +2,35 @@
namespace std {
-template< typename CharT >
+typedef int size_type;
+
+template
class basic_string {
public:
+ basic_string();
+ basic_string(const CharT *s);
+
~basic_string();
+ void clear();
+
+ basic_string =(const basic_string );
+ basic_string +=(const basic_string );
+
const CharT *c_str() const;
const CharT *data() const;
CharT *data();
+
+ basic_string (size_type count, CharT ch);
+ basic_string (size_type count, CharT ch);
+ basic_string (size_type index, size_type count);
+ basic_string (size_type index, size_type count, CharT ch);
+ basic_string (size_type pos, size_type count, const basic_string );
+ void pop_back();
+ void push_back(CharT ch);
+ void reserve(size_type new_cap);
+ void resize(size_type count);
+ void shrink_to_fit();
+ void swap(basic_string );
};
typedef basic_string string;
@@ -23,73 +45,70 @@
void consume(const char16_t *) {}
void consume(const char32_t *) {}
-void deref_after_scope_char_cstr() {
- const char *c;
+void deref_after_scope_char(bool cond) {
+ const char *c, *d;
{
std::string s;
c = s.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
+d = s.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+ }// expected-note {{Internal buffer is released because the object was destroyed}}
+ // expected-note@-1 {{Internal buffer is released because the object was destroyed}}
std::string s;
const char *c2 = s.c_str();
- consume(c); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
-}
-
-void deref_after_scope_char_data() {
- const char *c;
- {
-std::string s;
-c = s.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::string s;
- const char *c2 = s.data();
- consume(c); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
+ if (cond) {
+// expected-note@-1 {{Assuming 'cond' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'cond' is 0}}
+// expected-note@-4 {{Taking false branch}}
+consume(c); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ }
}
void deref_after_scope_char_data_non_const() {
char *c;
{
std::string s;
c = s.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
std::string s;
char *c2 = s.data();
consume(c); // expected-warning {{Use of memory after it is freed}}
// expected-note@-1 {{Use of memory after it is freed}}
}
-
-void deref_after_scope_wchar_t_cstr() {
- const wchar_t *w;
+void deref_after_scope_wchar_t(bool cond) {
+ const wchar_t *c, *d;
{
-std::wstring ws;
-w = ws.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::wstring ws;
- const wchar_t *w2 = ws.c_str();
- consume(w); // expected-warning {{Use of memory after it is freed}}
- // expected-note@-1 {{Use of memory after it is freed}}
-}
-
-void deref_after_scope_wchar_t_data() {
- const wchar_t *w;
- {
-std::wstring ws;
-w = ws.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
- } // expected-note {{Internal buffer is released because the object was destroyed}}
- std::wstring ws;
- const wchar_t *w2 = ws.data();
- consume(w); // expected-warning {{Use of memory
