Pierre-vh created this revision.
Pierre-vh added reviewers: dcoughlin, MaskRay.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus,
a.sidorin, szepet, baloghadamsoftware.
Herald added a reviewer: george.karpenkov.
Fix for the bug n°39792: False positive on strcpy targeting struct member
Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=39792
I fixed it by replacing the use of `dyn_cast` by two `isa`s to check if
`Target` is a `DeclRefExpr` or a `MemberExpr`.
The removal of the `DeclRef` variable seems to be meaningless because the only
place where the `DeclRef` variable was used is one line below, and it was used
to call a method which is inherited from Expr.
Thus, replacing the only use of `DeclRef` by `Target` should have no effect.
I also added a small test for this bugfix in
`test/Analysis/security-syntax-checks.m`
**Note:** I think we can completely remove the outer `if
(isa<DeclRefExpr>(Target) || isa<MemberExpr>(Target))`, no? Why should we only
allow `DeclRefExpr`s to pass this check?
**PS:** This is my first contribution ever to CLang (or any other open source
project), so I'm totally open to feedback, even if it's harsh.
Thank you for your attention!
Repository:
rC Clang
https://reviews.llvm.org/D55226
Files:
lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
test/Analysis/security-syntax-checks.m
Index: test/Analysis/security-syntax-checks.m
===================================================================
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -177,6 +177,11 @@
strcpy(x, "abcd");
}
+void test_strcpy_safe_2() {
+ struct {char s1[100];} s;
+ strcpy(s.s1, "hello");
+}
+
//===----------------------------------------------------------------------===
// strcat()
//===----------------------------------------------------------------------===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -651,8 +651,8 @@
const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
*Source = CE->getArg(1)->IgnoreImpCasts();
- if (const auto *DeclRef = dyn_cast<DeclRefExpr>(Target))
- if (const auto *Array = dyn_cast<ConstantArrayType>(DeclRef->getType())) {
+ if (isa<DeclRefExpr>(Target) || isa<MemberExpr>(Target))
+ if (const auto *Array = dyn_cast<ConstantArrayType>(Target->getType())) {
uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
if (const auto *String = dyn_cast<StringLiteral>(Source)) {
if (ArraySize >= String->getLength() + 1)
Index: test/Analysis/security-syntax-checks.m
===================================================================
--- test/Analysis/security-syntax-checks.m
+++ test/Analysis/security-syntax-checks.m
@@ -177,6 +177,11 @@
strcpy(x, "abcd");
}
+void test_strcpy_safe_2() {
+ struct {char s1[100];} s;
+ strcpy(s.s1, "hello");
+}
+
//===----------------------------------------------------------------------===
// strcat()
//===----------------------------------------------------------------------===
Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -651,8 +651,8 @@
const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
*Source = CE->getArg(1)->IgnoreImpCasts();
- if (const auto *DeclRef = dyn_cast<DeclRefExpr>(Target))
- if (const auto *Array = dyn_cast<ConstantArrayType>(DeclRef->getType())) {
+ if (isa<DeclRefExpr>(Target) || isa<MemberExpr>(Target))
+ if (const auto *Array = dyn_cast<ConstantArrayType>(Target->getType())) {
uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
if (const auto *String = dyn_cast<StringLiteral>(Source)) {
if (ArraySize >= String->getLength() + 1)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
