a.sidorin updated this revision to Diff 39198.
a.sidorin added a comment.
Thank you for you reply!
This version contains more radical solution. Also, I took an another look at
the CStringChecker and its way of handling checkDeadSymbols.
Repository:
rL LLVM
http://reviews.llvm.org/D14277
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
lib/StaticAnalyzer/Core/SymbolManager.cpp
test/Analysis/string.c
Index: test/Analysis/string.c
===================================================================
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -414,6 +414,12 @@
clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{TRUE}}
}
+void strcat_symbolic_src_length(char *src) {
+ char dst[8] = "1234";
+ strcat(dst, src);
+ clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{TRUE}}
+}
+
void strcat_symbolic_dst_length_taint(char *dst) {
scanf("%s", dst); // Taint data.
strcat(dst, "1234");
@@ -520,6 +526,17 @@
clang_analyzer_eval(strlen(x) > 4); // expected-warning{{UNKNOWN}}
}
+void strncpy_exactly_matching_buffer2(char *y) {
+ if (strlen(y) >= 4)
+ return;
+
+ char x[4];
+ strncpy(x, y, 4); // no-warning
+
+ // This time, we know that y fits in x anyway.
+ clang_analyzer_eval(strlen(x) <= 3); // expected-warning{{TRUE}}
+}
+
void strncpy_zero(char *src) {
char dst[] = "123";
strncpy(dst, src, 0); // no-warning
@@ -1079,30 +1096,3 @@
// character. For now, we just model the invalidation.
clang_analyzer_eval(str[1] == 'b'); // expected-warning{{UNKNOWN}}
}
-
-//===----------------------------------------------------------------------===
-// FIXMEs
-//===----------------------------------------------------------------------===
-
-// The analyzer_eval call below should evaluate to true. We are being too
-// aggressive in marking the (length of) src symbol dead. The length of dst
-// depends on src. This could be explicitely specified in the checker or the
-// logic for handling MetadataSymbol in SymbolManager needs to change.
-void strcat_symbolic_src_length(char *src) {
- char dst[8] = "1234";
- strcat(dst, src);
- clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{UNKNOWN}}
-}
-
-// The analyzer_eval call below should evaluate to true. Most likely the same
-// issue as the test above.
-void strncpy_exactly_matching_buffer2(char *y) {
- if (strlen(y) >= 4)
- return;
-
- char x[4];
- strncpy(x, y, 4); // no-warning
-
- // This time, we know that y fits in x anyway.
- clang_analyzer_eval(strlen(x) <= 3); // expected-warning{{UNKNOWN}}
-}
Index: lib/StaticAnalyzer/Core/SymbolManager.cpp
===================================================================
--- lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -393,11 +393,6 @@
RegionRoots.insert(region);
}
-void SymbolReaper::markInUse(SymbolRef sym) {
- if (isa<SymbolMetadata>(sym))
- MetadataInUse.insert(sym);
-}
-
bool SymbolReaper::maybeDead(SymbolRef sym) {
if (isLive(sym))
return false;
@@ -459,10 +454,7 @@
KnownLive = isLiveRegion(cast<SymbolExtent>(sym)->getRegion());
break;
case SymExpr::MetadataKind:
- KnownLive = MetadataInUse.count(sym) &&
- isLiveRegion(cast<SymbolMetadata>(sym)->getRegion());
- if (KnownLive)
- MetadataInUse.erase(sym);
+ KnownLive = isLiveRegion(cast<SymbolMetadata>(sym)->getRegion());
break;
case SymExpr::SymIntKind:
KnownLive = isLive(cast<SymIntExpr>(sym)->getLHS());
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -2123,28 +2123,22 @@
for (SymExpr::symbol_iterator si = Len.symbol_begin(),
se = Len.symbol_end(); si != se; ++si)
- SR.markInUse(*si);
+ SR.markLive(*si);
}
}
void CStringChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
- if (!SR.hasDeadSymbols())
- return;
-
- ProgramStateRef state = C.getState();
- CStringLengthTy Entries = state->get<CStringLength>();
+ auto state = C.getState();
+ auto Entries = state->get<CStringLength>();
if (Entries.isEmpty())
return;
- CStringLengthTy::Factory &F = state->get_context<CStringLength>();
- for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end();
- I != E; ++I) {
- SVal Len = I.getData();
- if (SymbolRef Sym = Len.getAsSymbol()) {
- if (SR.isDead(Sym))
- Entries = F.remove(Entries, I.getKey());
- }
+ auto &F = state->get_context<CStringLength>();
+ for (auto I = Entries.begin(), E = Entries.end(); I != E; ++I) {
+ const MemRegion *MR = I.getKey();
+ if (!SR.isLiveRegion(MR))
+ Entries = F.remove(Entries, MR);
}
state = state->set<CStringLength>(Entries);
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
@@ -565,7 +565,6 @@
typedef llvm::DenseSet<const MemRegion *> RegionSetTy;
SymbolMapTy TheLiving;
- SymbolSetTy MetadataInUse;
SymbolSetTy TheDead;
RegionSetTy RegionRoots;
@@ -603,15 +602,6 @@
/// environment. Checkers should instead use metadata symbols and markInUse.
void markLive(SymbolRef sym);
- /// \brief Marks a symbol as important to a checker.
- ///
- /// For metadata symbols,
- /// this will keep the symbol alive as long as its associated region is also
- /// live. For other symbols, this has no effect; checkers are not permitted
- /// to influence the life of other symbols. This should be used before any
- /// symbol marking has occurred, i.e. in the MarkLiveSymbols callback.
- void markInUse(SymbolRef sym);
-
/// \brief If a symbol is known to be live, marks the symbol as live.
///
/// Otherwise, if the symbol cannot be proven live, it is marked as dead.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
