[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
This revision was automatically updated to reflect the committed changes.
Closed by commit rC338259: [analyzer] Add support for more invalidating
functions in InnerPointerChecker. (authored by rkovacs, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D49656
Files:
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/inner-pointer.cpp
Index: test/Analysis/inner-pointer.cpp
===
--- test/Analysis/inner-pointer.cpp
+++ test/Analysis/inner-pointer.cpp
@@ -38,13 +38,29 @@
typedef basic_string u16string;
typedef basic_string u32string;
+template
+void func_ref(T &a);
+
+template
+void func_const_ref(const T &a);
+
+template
+void func_value(T a);
+
+string my_string = "default";
+void default_arg(int a = 42, string &b = my_string);
+
} // end namespace std
void consume(const char *) {}
void consume(const wchar_t *) {}
void consume(const char16_t *) {}
void consume(const char32_t *) {}
+//=--=//
+// `std::string` member functions //
+//=--=//
+
void deref_after_scope_char(bool cond) {
const char *c, *d;
{
@@ -151,6 +167,19 @@
} // expected-note@-1 {{Use of memory after it is freed}}
}
+void deref_after_scope_ok(bool cond) {
+ const char *c, *d;
+ std::string s;
+ {
+c = s.c_str();
+d = s.data();
+ }
+ if (cond)
+consume(c); // no-warning
+ else
+consume(d); // no-warning
+}
+
void deref_after_equals() {
const char *c;
std::string s = "hello";
@@ -277,15 +306,58 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
-void deref_after_scope_ok(bool cond) {
- const char *c, *d;
+//=---=//
+// Other STL functions //
+//=---=//
+
+void STL_func_ref() {
+ const char *c;
std::string s;
- {
-c = s.c_str();
-d = s.data();
- }
- if (cond)
-consume(c); // no-warning
- else
-consume(d); // no-warning
+ c = s.c_str();// expected-note {{Dangling inner pointer obtained here}}
+ std::func_ref(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c); // expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void STL_func_const_ref() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_const_ref(s);
+ consume(c); // no-warning
+}
+
+void STL_func_value() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_value(s);
+ consume(c); // no-warning
+}
+
+void func_ptr_known() {
+ const char *c;
+ std::string s;
+ void (*func_ptr)(std::string &) = std::func_ref;
+ c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+ func_ptr(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void func_ptr_unknown(void (*func_ptr)(std::string &)) {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ func_ptr(s);
+ consume(c); // no-warning
+}
+
+void func_default_arg() {
+ const char *c;
+ std::string s;
+ c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+ default_arg(3, s); // expected-note {{Inner pointer invalidated by call to 'default_arg'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
}
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -91,37 +91,53 @@
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 &Call) const;
-
- /// Record the connection between the symbol returned by c_str() and the
- /// corresponding string object region in the ProgramState. Mark the symbol
- /// released if the string object is destroyed.
+ /// Check if the object of this member function call is a `basic_string`.
+ bool isCalledOnStringObject(const CXXInstanceCall *ICall) const;
+
+ /// Check whether the called member function potentially invalidates
+ /// pointers referring to the container object's inner buffer.
+ bool isInvalidatingMemberFunction(const CallEvent &Call) const;
+
+ /// Mark pointer symbols associated with the given memory region released
+ /// in the program state.
+ void markPtrSymbolsReleased(const CallEvent &Call, ProgramStateRef Stat
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D49656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:192
- if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-SVal RawPtr = Call.getReturnValue();
-if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
- // Start tracking this raw pointer by adding it to the set of symbols
- // associated with this container object in the program state map.
- PtrSet::Factory &F = State->getStateManager().get_context();
- const PtrSet *SetPtr = State->get(ObjRegion);
- PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
- assert(C.wasInlined || !Set.contains(Sym));
- Set = F.add(Set, Sym);
- State = State->set(ObjRegion, Set);
- C.addTransition(State);
+ SVal Arg = FC->getArgSVal(ArgI);
+ const auto *ArgRegion =
rnkovacs wrote:
> NoQ wrote:
> > xazax.hun wrote:
> > > While I cannot recall examples in the STL the number of arguments and
> > > parameters might differ. We might have ellipsis in the parameters and
> > > this way we may pass more arguments. Since we do not have the parameter
> > > type for this case, I think it is ok to not handle it. But it might be
> > > worth to have a comment. The other case, when we have default arguments.
> > > I do not really know how the analyzer behaves with default arguments but
> > > maybe it would be worth to add a test case to ensure we do not crash on
> > > that?
> > The analyzer behaves pretty badly with default arguments; we don't really
> > model them unless they are like plain integer literals. But that's not an
> > issue here because a default parameter/argument is still both a parameter
> > and an argument (where the argument expression takes the form of
> > `CXXDefaultArgExpr`).
> Hm, it seems that passing objects of non-trivial types like `std::string` to
> a variadic function is implementation defined, and for `clang 6.0`, this
> means that it does not compile (does compile with `gcc 8.1` though, [[
> https://godbolt.org/g/C3MCtV | example ]]).
>
> Added a test for the default parameter case.
Yup, that's correct; i noticed it a few days ago too in
https://reviews.llvm.org/D49443#1170831.
You can disable the warning/error with `-Wno-non-pod-varargs` for the purposes
of testing, but you anyway won't be able to obtain the AST you'll want to test,
so i don't think this requires a test.
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:192
- if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-SVal RawPtr = Call.getReturnValue();
-if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
- // Start tracking this raw pointer by adding it to the set of symbols
- // associated with this container object in the program state map.
- PtrSet::Factory &F = State->getStateManager().get_context();
- const PtrSet *SetPtr = State->get(ObjRegion);
- PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
- assert(C.wasInlined || !Set.contains(Sym));
- Set = F.add(Set, Sym);
- State = State->set(ObjRegion, Set);
- C.addTransition(State);
+ SVal Arg = FC->getArgSVal(ArgI);
+ const auto *ArgRegion =
NoQ wrote:
> xazax.hun wrote:
> > While I cannot recall examples in the STL the number of arguments and
> > parameters might differ. We might have ellipsis in the parameters and this
> > way we may pass more arguments. Since we do not have the parameter type for
> > this case, I think it is ok to not handle it. But it might be worth to have
> > a comment. The other case, when we have default arguments. I do not really
> > know how the analyzer behaves with default arguments but maybe it would be
> > worth to add a test case to ensure we do not crash on that?
> The analyzer behaves pretty badly with default arguments; we don't really
> model them unless they are like plain integer literals. But that's not an
> issue here because a default parameter/argument is still both a parameter and
> an argument (where the argument expression takes the form of
> `CXXDefaultArgExpr`).
Hm, it seems that passing objects of non-trivial types like `std::string` to a
variadic function is implementation defined, and for `clang 6.0`, this means
that it does not compile (does compile with `gcc 8.1` though, [[
https://godbolt.org/g/C3MCtV | example ]]).
Added a test for the default parameter case.
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs updated this revision to Diff 157809.
rnkovacs marked an inline comment as done.
https://reviews.llvm.org/D49656
Files:
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/inner-pointer.cpp
Index: test/Analysis/inner-pointer.cpp
===
--- test/Analysis/inner-pointer.cpp
+++ test/Analysis/inner-pointer.cpp
@@ -38,13 +38,29 @@
typedef basic_string u16string;
typedef basic_string u32string;
+template
+void func_ref(T &a);
+
+template
+void func_const_ref(const T &a);
+
+template
+void func_value(T a);
+
+string my_string = "default";
+void default_arg(int a = 42, string &b = my_string);
+
} // end namespace std
void consume(const char *) {}
void consume(const wchar_t *) {}
void consume(const char16_t *) {}
void consume(const char32_t *) {}
+//=--=//
+// `std::string` member functions //
+//=--=//
+
void deref_after_scope_char(bool cond) {
const char *c, *d;
{
@@ -151,6 +167,19 @@
} // expected-note@-1 {{Use of memory after it is freed}}
}
+void deref_after_scope_ok(bool cond) {
+ const char *c, *d;
+ std::string s;
+ {
+c = s.c_str();
+d = s.data();
+ }
+ if (cond)
+consume(c); // no-warning
+ else
+consume(d); // no-warning
+}
+
void deref_after_equals() {
const char *c;
std::string s = "hello";
@@ -277,15 +306,58 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
-void deref_after_scope_ok(bool cond) {
- const char *c, *d;
+//=---=//
+// Other STL functions //
+//=---=//
+
+void STL_func_ref() {
+ const char *c;
std::string s;
- {
-c = s.c_str();
-d = s.data();
- }
- if (cond)
-consume(c); // no-warning
- else
-consume(d); // no-warning
+ c = s.c_str();// expected-note {{Dangling inner pointer obtained here}}
+ std::func_ref(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c); // expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void STL_func_const_ref() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_const_ref(s);
+ consume(c); // no-warning
+}
+
+void STL_func_value() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_value(s);
+ consume(c); // no-warning
+}
+
+void func_ptr_known() {
+ const char *c;
+ std::string s;
+ void (*func_ptr)(std::string &) = std::func_ref;
+ c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+ func_ptr(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void func_ptr_unknown(void (*func_ptr)(std::string &)) {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ func_ptr(s);
+ consume(c); // no-warning
+}
+
+void func_default_arg() {
+ const char *c;
+ std::string s;
+ c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+ default_arg(3, s); // expected-note {{Inner pointer invalidated by call to 'default_arg'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2930,6 +2930,11 @@
OS << MemCallE->getMethodDecl()->getNameAsString();
} else if (const auto *OpCallE = dyn_cast(S)) {
OS << OpCallE->getDirectCallee()->getNameAsString();
+} else if (const auto *CallE = dyn_cast(S)) {
+ auto &CEMgr = BRC.getStateManager().getCallEventManager();
+ CallEventRef<> Call = CEMgr.getSimpleCall(CallE, state, CurrentLC);
+ const auto *D = dyn_cast_or_null(Call->getDecl());
+ OS << (D ? D->getNameAsString() : "unknown");
}
OS << "'";
}
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -91,37 +91,53 @@
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 &Call) const;
-
- /// Record the connection between the symbol returned b
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
NoQ accepted this revision.
NoQ added a comment.
Looks great!
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:192
- if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-SVal RawPtr = Call.getReturnValue();
-if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
- // Start tracking this raw pointer by adding it to the set of symbols
- // associated with this container object in the program state map.
- PtrSet::Factory &F = State->getStateManager().get_context();
- const PtrSet *SetPtr = State->get(ObjRegion);
- PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
- assert(C.wasInlined || !Set.contains(Sym));
- Set = F.add(Set, Sym);
- State = State->set(ObjRegion, Set);
- C.addTransition(State);
+ SVal Arg = FC->getArgSVal(ArgI);
+ const auto *ArgRegion =
xazax.hun wrote:
> While I cannot recall examples in the STL the number of arguments and
> parameters might differ. We might have ellipsis in the parameters and this
> way we may pass more arguments. Since we do not have the parameter type for
> this case, I think it is ok to not handle it. But it might be worth to have a
> comment. The other case, when we have default arguments. I do not really know
> how the analyzer behaves with default arguments but maybe it would be worth
> to add a test case to ensure we do not crash on that?
The analyzer behaves pretty badly with default arguments; we don't really model
them unless they are like plain integer literals. But that's not an issue here
because a default parameter/argument is still both a parameter and an argument
(where the argument expression takes the form of `CXXDefaultArgExpr`).
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207-208
+
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
+ if (!ParamTy->isReferenceType() ||
rnkovacs wrote:
> NoQ wrote:
> > NoQ wrote:
> > > We need tests for operators here, due to the problem that i recently
> > > encountered in D49627: there's different numbering for arguments and
> > > parameters within in-class operators. Namely, this-argument is counted as
> > > an argument (shifting other argument indices by 1) but not as a parameter.
> > (i.e., tests and most likely some actual code branch)
> I did the coding part, but for the tests, I'm struggling to find an example
> of a member operator in the STL that takes a non-const string reference as an
> argument. Could you perhaps help me out here?
Mmm m right, dunno. I thought `operator>>` would be our main example, but
it's apparently non-member for a string, even though it is defined as a member
for primitive types.
I guess let's skip the test until we find an example. We did our best to work
around potential future problems, that's good.
Comment at: test/Analysis/inner-pointer.cpp:41-42
+template< class T >
+void func_ref(T& a);
+
rnkovacs wrote:
> NoQ wrote:
> > Without a definition for this thing, we won't be able to test much. I
> > suggest you to take a pointer to a function directly, i.e. define a
> > `std::swap(x, y)` somewhere and take a `&std::swap`
> > directly and call it (hope it actually works this way).
> I think I am missing something. This is meant to test that the checker
> recognizes standard library function calls taking a non-const reference to a
> string as an argument. At L314, `func_ref` is called with a string argument,
> and the checker seems to do all the things it should after the current patch,
> emitting the new kind of note and such.
>
> I can surely add the `std::swap(x, y)` definition too, but then the
> analyzer will see that the pointer was reallocated at the assignment inside
> the function, and place the note there. I think that would test the previous
> string-member-function patch more than this one.
Mm, aha, ok, i misunderstood this test. I thought you're trying to define
something like `std::function` and try to see if calling a function pointer
wrapped into that thing works. No idea why i was thinking that, sry.
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:181
- auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
- if (TypeDecl->getName() != "basic_string")
-return;
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
rnkovacs wrote:
> xazax.hun wrote:
> > Nit: maybe using `FunctionDecl::parameters` and range based for loop is
> > sligtly cleaner.
> Given that I am manipulating indices below, I feel that actually the plain
> for loop is a bit simpler here, what do you think?
Indeed, I am fine with the old style loop too.
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:181
- auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
- if (TypeDecl->getName() != "basic_string")
-return;
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
xazax.hun wrote:
> Nit: maybe using `FunctionDecl::parameters` and range based for loop is
> sligtly cleaner.
Given that I am manipulating indices below, I feel that actually the plain for
loop is a bit simpler here, what do you think?
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs updated this revision to Diff 157286.
rnkovacs marked an inline comment as done.
rnkovacs added a comment.
Tiny bit more re-structuring.
https://reviews.llvm.org/D49656
Files:
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/inner-pointer.cpp
Index: test/Analysis/inner-pointer.cpp
===
--- test/Analysis/inner-pointer.cpp
+++ test/Analysis/inner-pointer.cpp
@@ -38,13 +38,26 @@
typedef basic_string u16string;
typedef basic_string u32string;
+template
+void func_ref(T &a);
+
+template
+void func_const_ref(const T &a);
+
+template
+void func_value(T a);
+
} // end namespace std
void consume(const char *) {}
void consume(const wchar_t *) {}
void consume(const char16_t *) {}
void consume(const char32_t *) {}
+//=--=//
+// `std::string` member functions //
+//=--=//
+
void deref_after_scope_char(bool cond) {
const char *c, *d;
{
@@ -151,6 +164,19 @@
} // expected-note@-1 {{Use of memory after it is freed}}
}
+void deref_after_scope_ok(bool cond) {
+ const char *c, *d;
+ std::string s;
+ {
+c = s.c_str();
+d = s.data();
+ }
+ if (cond)
+consume(c); // no-warning
+ else
+consume(d); // no-warning
+}
+
void deref_after_equals() {
const char *c;
std::string s = "hello";
@@ -277,15 +303,49 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
-void deref_after_scope_ok(bool cond) {
- const char *c, *d;
+//=---=//
+// Other STL functions //
+//=---=//
+
+void STL_func_ref() {
+ const char *c;
std::string s;
- {
-c = s.c_str();
-d = s.data();
- }
- if (cond)
-consume(c); // no-warning
- else
-consume(d); // no-warning
+ c = s.c_str();// expected-note {{Dangling inner pointer obtained here}}
+ std::func_ref(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c); // expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void STL_func_const_ref() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_const_ref(s);
+ consume(c); // no-warning
+}
+
+void STL_func_value() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_value(s);
+ consume(c); // no-warning
+}
+
+void func_ptr_known() {
+ const char *c;
+ std::string s;
+ void (*func_ptr)(std::string &) = std::func_ref;
+ c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+ func_ptr(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void func_ptr_unknown(void (*func_ptr)(std::string &)) {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ func_ptr(s);
+ consume(c); // no-warning
}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2930,6 +2930,11 @@
OS << MemCallE->getMethodDecl()->getNameAsString();
} else if (const auto *OpCallE = dyn_cast(S)) {
OS << OpCallE->getDirectCallee()->getNameAsString();
+} else if (const auto *CallE = dyn_cast(S)) {
+ auto &CEMgr = BRC.getStateManager().getCallEventManager();
+ CallEventRef<> Call = CEMgr.getSimpleCall(CallE, state, CurrentLC);
+ const auto *D = dyn_cast_or_null(Call->getDecl());
+ OS << (D ? D->getNameAsString() : "unknown");
}
OS << "'";
}
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -91,37 +91,53 @@
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 &Call) const;
-
- /// Record the connection between the symbol returned by c_str() and the
- /// corresponding string object region in the ProgramState. Mark the symbol
- /// released if the string object is destroyed.
+ /// Check if the object of this member function call is a `basic_string`.
+ bool isCalledOnStringObject(const CXXInstanceCall *ICall) const;
+
+ /// Check whether the called member function potentially invalidates
+ /// pointers referring to the containe
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun added a comment.
Small comments inline.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:181
- auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
- if (TypeDecl->getName() != "basic_string")
-return;
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
Nit: maybe using `FunctionDecl::parameters` and range based for loop is sligtly
cleaner.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:189
+ // argument but not as a parameter.
+ const auto *MemberOpCall = dyn_cast(FC);
+ unsigned ArgI = MemberOpCall ? I+1 : I;
Nit: maybe using isa + bool is cleaner here?
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:192
- if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-SVal RawPtr = Call.getReturnValue();
-if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
- // Start tracking this raw pointer by adding it to the set of symbols
- // associated with this container object in the program state map.
- PtrSet::Factory &F = State->getStateManager().get_context();
- const PtrSet *SetPtr = State->get(ObjRegion);
- PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
- assert(C.wasInlined || !Set.contains(Sym));
- Set = F.add(Set, Sym);
- State = State->set(ObjRegion, Set);
- C.addTransition(State);
+ SVal Arg = FC->getArgSVal(ArgI);
+ const auto *ArgRegion =
While I cannot recall examples in the STL the number of arguments and
parameters might differ. We might have ellipsis in the parameters and this way
we may pass more arguments. Since we do not have the parameter type for this
case, I think it is ok to not handle it. But it might be worth to have a
comment. The other case, when we have default arguments. I do not really know
how the analyzer behaves with default arguments but maybe it would be worth to
add a test case to ensure we do not crash on that?
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207-208
+
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
+ if (!ParamTy->isReferenceType() ||
NoQ wrote:
> NoQ wrote:
> > We need tests for operators here, due to the problem that i recently
> > encountered in D49627: there's different numbering for arguments and
> > parameters within in-class operators. Namely, this-argument is counted as
> > an argument (shifting other argument indices by 1) but not as a parameter.
> (i.e., tests and most likely some actual code branch)
I did the coding part, but for the tests, I'm struggling to find an example of
a member operator in the STL that takes a non-const string reference as an
argument. Could you perhaps help me out here?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2934
+} else if (const auto *CallE = dyn_cast(S)) {
+ OS << CallE->getDirectCallee()->getNameAsString();
}
NoQ wrote:
> rnkovacs wrote:
> > xazax.hun wrote:
> > > I think `getDirectCallee` might fail and return `nullptr`. One more
> > > reason to test function pointers :)
> > You're right. Also, it needed a bit more effort to dig up the function
> > pointer's name. Or should I go further and somehow find out the name of the
> > function it points to?
> > Also, it needed a bit more effort to dig up the function pointer's name.
>
> I think it should work out of the box; `Call.getDecl()` is smart enough to
> show the right decl when a pointer to a concrete function is being called on
> the current path.
That worked, thanks!
Comment at: test/Analysis/inner-pointer.cpp:41-42
+template< class T >
+void func_ref(T& a);
+
NoQ wrote:
> Without a definition for this thing, we won't be able to test much. I suggest
> you to take a pointer to a function directly, i.e. define a `std::swap(x,
> y)` somewhere and take a `&std::swap` directly and call it (hope
> it actually works this way).
I think I am missing something. This is meant to test that the checker
recognizes standard library function calls taking a non-const reference to a
string as an argument. At L314, `func_ref` is called with a string argument,
and the checker seems to do all the things it should after the current patch,
emitting the new kind of note and such.
I can surely add the `std::swap(x, y)` definition too, but then the analyzer
will see that the pointer was reallocated at the assignment inside the
function, and place the note there. I think that would test the previous
string-member-function patch more than this one.
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs updated this revision to Diff 157278.
rnkovacs marked 2 inline comments as done.
rnkovacs added a comment.
Fix note for function pointers & handle argument counting in member operator
calls.
I also refactored the code a little, because after moving things from
`checkPreCall` to `checkPostCall`, the structure was a bit confusing.
https://reviews.llvm.org/D49656
Files:
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/inner-pointer.cpp
Index: test/Analysis/inner-pointer.cpp
===
--- test/Analysis/inner-pointer.cpp
+++ test/Analysis/inner-pointer.cpp
@@ -38,13 +38,26 @@
typedef basic_string u16string;
typedef basic_string u32string;
+template
+void func_ref(T &a);
+
+template
+void func_const_ref(const T &a);
+
+template
+void func_value(T a);
+
} // end namespace std
void consume(const char *) {}
void consume(const wchar_t *) {}
void consume(const char16_t *) {}
void consume(const char32_t *) {}
+//=--=//
+// `std::string` member functions //
+//=--=//
+
void deref_after_scope_char(bool cond) {
const char *c, *d;
{
@@ -151,6 +164,19 @@
} // expected-note@-1 {{Use of memory after it is freed}}
}
+void deref_after_scope_ok(bool cond) {
+ const char *c, *d;
+ std::string s;
+ {
+c = s.c_str();
+d = s.data();
+ }
+ if (cond)
+consume(c); // no-warning
+ else
+consume(d); // no-warning
+}
+
void deref_after_equals() {
const char *c;
std::string s = "hello";
@@ -277,15 +303,49 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
-void deref_after_scope_ok(bool cond) {
- const char *c, *d;
+//=---=//
+// Other STL functions //
+//=---=//
+
+void STL_func_ref() {
+ const char *c;
std::string s;
- {
-c = s.c_str();
-d = s.data();
- }
- if (cond)
-consume(c); // no-warning
- else
-consume(d); // no-warning
+ c = s.c_str();// expected-note {{Dangling inner pointer obtained here}}
+ std::func_ref(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c); // expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void STL_func_const_ref() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_const_ref(s);
+ consume(c); // no-warning
+}
+
+void STL_func_value() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_value(s);
+ consume(c); // no-warning
+}
+
+void func_ptr_known() {
+ const char *c;
+ std::string s;
+ void (*func_ptr)(std::string &) = std::func_ref;
+ c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+ func_ptr(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void func_ptr_unknown(void (*func_ptr)(std::string &)) {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ func_ptr(s);
+ consume(c); // no-warning
}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2930,6 +2930,11 @@
OS << MemCallE->getMethodDecl()->getNameAsString();
} else if (const auto *OpCallE = dyn_cast(S)) {
OS << OpCallE->getDirectCallee()->getNameAsString();
+} else if (const auto *CallE = dyn_cast(S)) {
+ auto &CEMgr = BRC.getStateManager().getCallEventManager();
+ CallEventRef<> Call = CEMgr.getSimpleCall(CallE, state, CurrentLC);
+ const auto *D = dyn_cast_or_null(Call->getDecl());
+ OS << (D ? D->getNameAsString() : "unknown");
}
OS << "'";
}
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -91,37 +91,53 @@
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 &Call) const;
-
- /// Record the connection between the symbol returned by c_str() and the
- /// corresponding string object region in the ProgramState. Mark the symbol
- /// released if the string object is destroyed.
+ /// Check if the object of this member function call is a `ba
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207-208
+
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
+ if (!ParamTy->isReferenceType() ||
NoQ wrote:
> We need tests for operators here, due to the problem that i recently
> encountered in D49627: there's different numbering for arguments and
> parameters within in-class operators. Namely, this-argument is counted as an
> argument (shifting other argument indices by 1) but not as a parameter.
(i.e., tests and most likely some actual code branch)
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207-208
+
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
+ if (!ParamTy->isReferenceType() ||
We need tests for operators here, due to the problem that i recently
encountered in D49627: there's different numbering for arguments and parameters
within in-class operators. Namely, this-argument is counted as an argument
(shifting other argument indices by 1) but not as a parameter.
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
NoQ added inline comments. Comment at: test/Analysis/inner-pointer.cpp:41-42 +template< class T > +void func_ref(T& a); + Without a definition for this thing, we won't be able to test much. I suggest you to take a pointer to a function directly, i.e. define a `std::swap(x, y)` somewhere and take a `&std::swap` directly and call it (hope it actually works this way). https://reviews.llvm.org/D49656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2934
+} else if (const auto *CallE = dyn_cast(S)) {
+ OS << CallE->getDirectCallee()->getNameAsString();
}
rnkovacs wrote:
> xazax.hun wrote:
> > I think `getDirectCallee` might fail and return `nullptr`. One more reason
> > to test function pointers :)
> You're right. Also, it needed a bit more effort to dig up the function
> pointer's name. Or should I go further and somehow find out the name of the
> function it points to?
> Also, it needed a bit more effort to dig up the function pointer's name.
I think it should work out of the box; `Call.getDecl()` is smart enough to show
the right decl when a pointer to a concrete function is being called on the
current path.
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:213
+ if (const auto *FC = dyn_cast(&Call)) {
+const FunctionDecl *FD = FC->getDecl();
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
xazax.hun wrote:
> I am not sure if we always have a `Decl` here, I am afraid this might return
> null sometimes. Please add a test case with a function pointer (received as
> an argument in a top level function).
Um, yes, I'd already seen a crash here on a project before I saw your comment.
Thanks!
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2934
+} else if (const auto *CallE = dyn_cast(S)) {
+ OS << CallE->getDirectCallee()->getNameAsString();
}
xazax.hun wrote:
> I think `getDirectCallee` might fail and return `nullptr`. One more reason to
> test function pointers :)
You're right. Also, it needed a bit more effort to dig up the function
pointer's name. Or should I go further and somehow find out the name of the
function it points to?
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs updated this revision to Diff 156938.
rnkovacs marked 11 inline comments as done.
rnkovacs added a comment.
Addressed comments & added two test cases for function pointers.
https://reviews.llvm.org/D49656
Files:
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/inner-pointer.cpp
Index: test/Analysis/inner-pointer.cpp
===
--- test/Analysis/inner-pointer.cpp
+++ test/Analysis/inner-pointer.cpp
@@ -38,6 +38,15 @@
typedef basic_string u16string;
typedef basic_string u32string;
+template< class T >
+void func_ref(T& a);
+
+template< class T >
+void func_const_ref(const T& a);
+
+template< class T >
+void func_value(T a);
+
} // end namespace std
void consume(const char *) {}
@@ -277,6 +286,49 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
+void STL_func_ref() {
+ const char *c;
+ std::string s;
+ c = s.c_str();// expected-note {{Dangling inner pointer obtained here}}
+ std::func_ref(s); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void STL_func_const_ref() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_const_ref(s);
+ consume(c); // no-warning
+}
+
+void STL_func_value() {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ std::func_value(s);
+ consume(c); // no-warning
+}
+
+void func_ptr_known() {
+ const char *c;
+ std::string s;
+ void (*func_ptr)(std::string&) = std::func_ref;
+ c = s.c_str(); // expected-note {{Dangling inner pointer obtained here}}
+ func_ptr(s); // expected-note {{Inner pointer invalidated by call to 'func_ptr'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void func_ptr_unknown(void (*func_ptr)(std::string&)) {
+ const char *c;
+ std::string s;
+ c = s.c_str();
+ func_ptr(s);
+ consume(c); // no-warning
+}
+
void deref_after_scope_ok(bool cond) {
const char *c, *d;
std::string s;
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2930,6 +2930,9 @@
OS << MemCallE->getMethodDecl()->getNameAsString();
} else if (const auto *OpCallE = dyn_cast(S)) {
OS << OpCallE->getDirectCallee()->getNameAsString();
+} else if (const auto *CallE = dyn_cast(S)) {
+ const auto *D = dyn_cast_or_null(CallE->getCalleeDecl());
+ OS << (D ? D->getNameAsString() : "unknown");
}
OS << "'";
}
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -91,37 +91,29 @@
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 &Call) const;
-
- /// Record the connection between the symbol returned by c_str() and the
- /// corresponding string object region in the ProgramState. Mark the symbol
- /// released if the string object is destroyed.
+ /// Check whether the called member function potentially invalidates
+ /// pointers referring to the container object's inner buffer.
+ bool isInvalidatingMemberFunction(const CallEvent &Call) const;
+
+ /// Mark pointer symbols associated with the given memory region released
+ /// in the program state.
+ void markPtrSymbolsReleased(const CallEvent &Call, ProgramStateRef State,
+ const MemRegion *ObjRegion,
+ CheckerContext &C) const;
+
+ /// Record the connection between raw pointers referring to a container
+ /// object's inner buffer and the object's memory region in the program state.
+ /// Mark potentially invalidated pointers released.
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
- /// Clean up the ProgramState map.
+ /// Clean up the program state map.
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
};
} // 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 no
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207
- 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();
- for (const auto Symbol : *PS) {
-// NOTE: `Origin` may be null, and will be stored so in the symbol's
-// `RefState` in MallocChecker's `RegionState` program state map.
-State = allocation_state::markReleased(State, Symbol, Origin);
- }
- State = State->remove(ObjRegion);
- C.addTransition(State);
- return;
+void InnerPointerChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
NoQ wrote:
> I believe that this should also go into `PostCall`. Symbols aren't released
> until some point //within// the call.
Oh, I see. But is it guaranteed that the symbols are not garbage collected
until post call? Also, the environment will always contain all the bindings for
the arguments?
Repository:
rC Clang
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
NoQ added a comment.
I've just one thing to add.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:149
+C.addTransition(State);
return;
+ }
xazax.hun wrote:
> Nit: This return is redundant.
Because of how easy it is to accidentally split the state, i'm on a brink of
declaring `return` after `addTransition` a good practice.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207
- 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();
- for (const auto Symbol : *PS) {
-// NOTE: `Origin` may be null, and will be stored so in the symbol's
-// `RefState` in MallocChecker's `RegionState` program state map.
-State = allocation_state::markReleased(State, Symbol, Origin);
- }
- State = State->remove(ObjRegion);
- C.addTransition(State);
- return;
+void InnerPointerChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
I believe that this should also go into `PostCall`. Symbols aren't released
until some point //within// the call.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:212
+ // Check [string.require] / first point.
+ if (const auto *FC = dyn_cast(&Call)) {
+const FunctionDecl *FD = FC->getDecl();
xazax.hun wrote:
> Shouldn't we also check if the function is a standard library function? Or do
> we assume that user functions also invalidate the strings?
That's right, it's an important thing to check.
Repository:
rC Clang
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun requested changes to this revision.
xazax.hun added a comment.
This revision now requires changes to proceed.
Some comments, mostly nits inline.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:149
+C.addTransition(State);
return;
+ }
Nit: This return is redundant.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:202
+ markPtrSymbolsReleased(Call, State, ObjRegion, C);
}
}
Nit: no need for braces here.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:204
}
+ return;
+}
Nit: redundant return.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:212
+ // Check [string.require] / first point.
+ if (const auto *FC = dyn_cast(&Call)) {
+const FunctionDecl *FD = FC->getDecl();
Shouldn't we also check if the function is a standard library function? Or do
we assume that user functions also invalidate the strings?
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:213
+ if (const auto *FC = dyn_cast(&Call)) {
+const FunctionDecl *FD = FC->getDecl();
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
I am not sure if we always have a `Decl` here, I am afraid this might return
null sometimes. Please add a test case with a function pointer (received as an
argument in a top level function).
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:227
}
+ return;
}
Nit: redundant return.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2934
+} else if (const auto *CallE = dyn_cast(S)) {
+ OS << CallE->getDirectCallee()->getNameAsString();
}
I think `getDirectCallee` might fail and return `nullptr`. One more reason to
test function pointers :)
Repository:
rC Clang
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet,
baloghadamsoftware, whisperity.
According to the standard, pointers referring to the elements of a
`basic_string` sequence may also be invalidated if they are used as an argument
to any standard library function taking a reference to non-const `basic_string`
as an argument. This patch makes InnerPointerChecker warn for these cases as
well.
Repository:
rC Clang
https://reviews.llvm.org/D49656
Files:
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/inner-pointer.cpp
Index: test/Analysis/inner-pointer.cpp
===
--- test/Analysis/inner-pointer.cpp
+++ test/Analysis/inner-pointer.cpp
@@ -38,6 +38,15 @@
typedef basic_string u16string;
typedef basic_string u32string;
+template< class T >
+void func_ref(T& a, T& b);
+
+template< class T >
+void func_const_ref(const T& a, const T& b);
+
+template< class T >
+void func_value(T a, T b);
+
} // end namespace std
void consume(const char *) {}
@@ -277,6 +286,31 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
+void non_member_func_ref() {
+ const char *c;
+ std::string s1, s2;
+ c = s1.c_str();// expected-note {{Dangling inner pointer obtained here}}
+ std::func_ref(s1, s2); // expected-note {{Inner pointer invalidated by call to 'func_ref'}}
+ consume(c);// expected-warning {{Use of memory after it is freed}}
+ // expected-note@-1 {{Use of memory after it is freed}}
+}
+
+void non_member_func_const_ref() {
+ const char *c;
+ std::string s1, s2;
+ c = s1.c_str();
+ std::func_const_ref(s1, s2);
+ consume(c); // no-warning
+}
+
+void non_member_func_value() {
+ const char *c;
+ std::string s1, s2;
+ c = s1.c_str();
+ std::func_value(s1, s2);
+ consume(c); // no-warning
+}
+
void deref_after_scope_ok(bool cond) {
const char *c, *d;
std::string s;
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2930,6 +2930,8 @@
OS << MemCallE->getMethodDecl()->getNameAsString();
} else if (const auto *OpCallE = dyn_cast(S)) {
OS << OpCallE->getDirectCallee()->getNameAsString();
+} else if (const auto *CallE = dyn_cast(S)) {
+ OS << CallE->getDirectCallee()->getNameAsString();
}
OS << "'";
}
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -45,7 +45,7 @@
namespace {
class InnerPointerChecker
-: public Checker {
+: public Checker {
CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
@@ -91,37 +91,33 @@
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 &Call) const;
+ /// Check whether the called member function potentially invalidates
+ /// pointers referring to the container object's inner buffer.
+ bool isInvalidatingMemberFunction(const CallEvent &Call) const;
- /// Record the connection between the symbol returned by c_str() and the
- /// corresponding string object region in the ProgramState. Mark the symbol
- /// released if the string object is destroyed.
+ /// Mark pointer symbols associated with the given memory region released
+ /// in the program state.
+ void markPtrSymbolsReleased(const CallEvent &Call, ProgramStateRef State,
+ const MemRegion *ObjRegion,
+ CheckerContext &C) const;
+
+ /// Record the connection between raw pointers referring to a container
+ /// object's inner buffer and the object's memory region in the program state.
+ /// Mark pointers potentially invalidated by member functions released.
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
- /// Clean up the ProgramState map.
+ /// Mark pointers potentially invalidated by functions taking the object
+ /// by non-const reference released.
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+
+ /// Clean up the program state map.
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
};
} // end anonymous namespace
-// [string.require]
-//
-// "References, p
