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

[Pierre-vh]

Summary

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 isas 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 DeclRefExprs 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!

[george.karpenkov]

Thank you for the fix, but how far can the pattern matching go? Seems easy enough to think of cases not covered by the above.
In any case, the fix looks good.

[Pierre-vh]

In D55226#1317083, @george.karpenkov wrote:

Thank you for the fix, but how far can the pattern matching go? Seems easy enough to think of cases not covered by the above.
In any case, the fix looks good.

Hey,

Sadly I'm not experienced enough to think of every case that should pass this check, so I limited myself to just fixing the bug.
Can't we totally remove the outer if so we allow every Target expression that has a ConstantArrayType to pass this check?

Thank you for your time!

[MTC]

Please add more context using the -U option, like git diff -U99999 ....

In D55226#1317119, @Pierre-vh wrote:

Sadly I'm not experienced enough to think of every case that should pass this check, so I limited myself to just fixing the bug.
Can't we totally remove the outer if so we allow every Target expression that has a ConstantArrayType to pass this check?

IMHO, it's fine to remove the outer check!

[Pierre-vh]

Hello again! I updated the diff and completely removed the outer if. Please let me know what you think!

[Szelethus]

Thanks for the fix! :)

*deleted rest of the comment after re-reading the code*

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

662 ↗

(On Diff #176661)

No need for this newline.

[Pierre-vh]

Here's the diff without the extra newline :)

[Pierre-vh]

Hi again!

As I'm quite new to this, I don't know what the next step is. Do we need to wait for more people to review this diff?
What happens when it's considered "ready"? How is it committed? (I don't have commit access)

Thank you for your help!

[Szelethus]

I like what I'm seeing.

In D55226#1320281, @Pierre-vh wrote:

Hi again!

As I'm quite new to this, I don't know what the next step is. Do we need to wait for more people to review this diff?
What happens when it's considered "ready"? How is it committed? (I don't have commit access)

Welcome! :) Always great to see new people around.

Once the revision is accepted (and, preferably accepted by someone knowledgeable in the analyzer) you may commit. When you start contributing to the project, and you don't have commit access just yet, you should ask someone who does to commit the patch on your behalf. Once you have "enough" patches under your belt, you may ask for a commit access.

Here are some links with more info on patch reviews, commiting, and the like:

https://llvm.org/docs/Contributing.html
https://llvm.org/docs/DeveloperPolicy.html ("Making and Submitting a Patch" seems outdated, but the following sections are worth a read: "Code Reviews", "Obtaining Commit Access" (and the rest, if you have the time))
https://llvm.org/docs/Phabricator.html

Sadly, I won't be around my working PC for a while, so I can't commit on your behalf just yet. If you asked for someone to commit but no one did, it's perfectly fine to ping it once a week (or, more frequently if it's urgent).

[Pierre-vh]

Hello!

I'm pinging since it's been a week. If someone can commit this patch on my behalf, that would be great.

Thank you :)

[Pierre-vh]

Hello!

I'm trying one last ping since it's been a month and it hasn't been commited (I think).

[george.karpenkov]

Whoops, sorry. There were holidays, and then I did forget about this patch. I'll commit this now.

[george.karpenkov]

@Pierre-vh The patch does not compile due to unmatched braces. Please do test and compile before submitting!

cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

void WalkAST::checkCall_strcpy(const CallExpr *CE, const FunctionDecl *FD) {
   if (!filter.check_strcpy)
     return;
 
   if (!checkCall_strCommon(CE, FD))
     return;
 
   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())) {
-      uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
-      if (const auto *String = dyn_cast<StringLiteral>(Source)) {
-        if (ArraySize >= String->getLength() + 1)
-          return;
-      }
-    }
+
+  if (const auto *Array = dyn_cast<ConstantArrayType>(Target->getType())) {
 
   // Issue a warning.
   PathDiagnosticLocation CELoc =
     PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
   BR.EmitBasicReport(AC->getDecl(), filter.checkName_strcpy,
                      "Potential insecure memory buffer bounds restriction in "
                      "call 'strcpy'",
                      "Security",

cfe/trunk/test/Analysis/security-syntax-checks.m

void test_strcpy_2() {
   strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
 }
 
 void test_strcpy_safe() {
   char x[5];
   strcpy(x, "abcd");
 }
 
+void test_strcpy_safe_2() {
+  struct {char s1[100];} s;
+  strcpy(s.s1, "hello");
+}
+
 //===----------------------------------------------------------------------===
 // strcat()
 //===----------------------------------------------------------------------===
 #ifdef VARIANT
 
 #define __strcat_chk BUILTIN(__strcat_chk)
 char *__strcat_chk(char *restrict s1, const char *restrict s2, size_t destlen);