[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[Pierre van Houtryve via Phabricator via cfe-commits]

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)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55226.176423.patch
Type: text/x-patch
Size: 1361 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181203/a94f6054/attachment-0001.bin>