[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[David CARLIER via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348884: [analyzer][CStringChecker] evaluate explicit_bzero 
(authored by devnexen, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54592?vs=177171=177739#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  cfe/trunk/test/Analysis/string.c

Index: cfe/trunk/test/Analysis/string.c
===
--- cfe/trunk/test/Analysis/string.c
+++ cfe/trunk/test/Analysis/string.c
@@ -1184,11 +1184,14 @@
 }
 
 //===--===
-// memset()
+// memset() / explicit_bzero() / bzero()
 //===--===
 
 void *memset(void *dest, int ch, size_t count);
 
+void bzero(void *dst, size_t count);
+void explicit_bzero(void *dest, size_t count);
+
 void *malloc(size_t size);
 void free(void *);
 
@@ -1383,6 +1386,57 @@
   clang_analyzer_eval(array[4] == 0); // expected-warning{{TRUE}}
 }
 
+void bzero1_null() {
+  char *a = NULL;
+
+  bzero(a, 10); // expected-warning{{Null pointer argument in call to memory clearance function}}
+}
+
+void bzero2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void bzero3_char_ptr_null() {
+  char *str = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str + 2, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{FALSE}}
+}
+
+void explicit_bzero1_null() {
+  char *a = NULL;
+
+  explicit_bzero(a, 10); // expected-warning{{Null pointer argument in call to memory clearance function}}
+}
+
+void explicit_bzero2_clear_mypassword() {
+  char passwd[7] = "passwd";
+
+  explicit_bzero(passwd, sizeof(passwd)); // no-warning
+
+  clang_analyzer_eval(strlen(passwd) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(passwd[0] == '\0'); // expected-warning{{TRUE}}
+}
+
+void explicit_bzero3_out_ofbound() {
+  char *privkey = (char *)malloc(7);
+  const char newprivkey[10] = "mysafekey";
+
+  strcpy(privkey, "random");
+  explicit_bzero(privkey, sizeof(newprivkey));
+#ifndef SUPPRESS_OUT_OF_BOUND
+  // expected-warning@-2 {{Memory clearance function accesses out-of-bound array element}}
+#endif
+  clang_analyzer_eval(privkey[0] == '\0');
+#ifdef SUPPRESS_OUT_OF_BOUND
+  // expected-warning@-2 {{UNKNOWN}}
+#endif
+  free(privkey);
+}
+
 //===--===
 // FIXMEs
 //===--===
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -124,6 +124,7 @@
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
   void evalMemset(CheckerContext , const CallExpr *CE) const;
+  void evalBzero(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -158,7 +159,7 @@
   static bool SummarizeRegion(raw_ostream , ASTContext ,
   const MemRegion *MR);
 
-  static bool memsetAux(const Expr *DstBuffer, const Expr *CharE,
+  static bool memsetAux(const Expr *DstBuffer, SVal CharE,
 const Expr *Size, CheckerContext ,
 ProgramStateRef );
 
@@ -1005,11 +1006,10 @@
   }
 }
 
-bool CStringChecker::memsetAux(const Expr *DstBuffer, const Expr *CharE,
+bool CStringChecker::memsetAux(const Expr *DstBuffer, SVal CharVal,
const Expr *Size, CheckerContext ,
ProgramStateRef ) {
   SVal MemVal = C.getSVal(DstBuffer);
-  SVal CharVal = C.getSVal(CharE);
   SVal SizeVal = C.getSVal(Size);
   const MemRegion *MR = MemVal.getAsRegion();
   if (!MR)
@@ -2184,13 +2184,59 @@
   // According to the values of the arguments, bind the value of the second
   // argument to the destination buffer and set string length, or just
   // invalidate the destination buffer.
-  if (!memsetAux(Mem, CharE, Size, C, State))
+  if (!memsetAux(Mem, C.getSVal(CharE), Size, C, State))
 return;
 
   State = State->BindExpr(CE, LCtx, MemVal);
   C.addTransition(State);
 }
 
+void CStringChecker::evalBzero(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() != 2)
+return;
+
+  CurrentFunctionDescription = "memory clearance function";
+
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Size = 

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Looks good, thanks!




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2216
+  // In this case we just return.
+  if (StateZeroSize && !StateNonZeroSize) {
+C.addTransition(StateZeroSize);

`!StateNonZeroSize` implies `StateZeroSize`, you can drop the left-hand side of 
`&&`.



Comment at: test/Analysis/string.c:1405-1406
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str + 2, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{FALSE}}
+}

devnexen wrote:
> NoQ wrote:
> > Let's also add the true statement. I.e., do we know here that the actual 
> > length is 2?
> I think that s the limit of this checker (even with memset that does not 
> work).
I mean, even if it doesn't work, let's add a FIXME test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[David CARLIER via Phabricator via cfe-commits]

devnexen added a comment.

ping just want to put it behind :-) thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[David CARLIER via Phabricator via cfe-commits]

devnexen updated this revision to Diff 177171.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1184,11 +1184,14 @@
 }
 
 //===--===
-// memset()
+// memset() / explicit_bzero() / bzero()
 //===--===
 
 void *memset(void *dest, int ch, size_t count);
 
+void bzero(void *dst, size_t count);
+void explicit_bzero(void *dest, size_t count);
+
 void *malloc(size_t size);
 void free(void *);
 
@@ -1383,6 +1386,57 @@
   clang_analyzer_eval(array[4] == 0); // expected-warning{{TRUE}}
 }
 
+void bzero1_null() {
+  char *a = NULL;
+
+  bzero(a, 10); // expected-warning{{Null pointer argument in call to memory clearance function}}
+}
+
+void bzero2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void bzero3_char_ptr_null() {
+  char *str = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str + 2, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{FALSE}}
+}
+
+void explicit_bzero1_null() {
+  char *a = NULL;
+
+  explicit_bzero(a, 10); // expected-warning{{Null pointer argument in call to memory clearance function}}
+}
+
+void explicit_bzero2_clear_mypassword() {
+  char passwd[7] = "passwd";
+
+  explicit_bzero(passwd, sizeof(passwd)); // no-warning
+
+  clang_analyzer_eval(strlen(passwd) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(passwd[0] == '\0'); // expected-warning{{TRUE}}
+}
+
+void explicit_bzero3_out_ofbound() {
+  char *privkey = (char *)malloc(7);
+  const char newprivkey[10] = "mysafekey";
+
+  strcpy(privkey, "random");
+  explicit_bzero(privkey, sizeof(newprivkey));
+#ifndef SUPPRESS_OUT_OF_BOUND
+  // expected-warning@-2 {{Memory clearance function accesses out-of-bound array element}}
+#endif
+  clang_analyzer_eval(privkey[0] == '\0');
+#ifdef SUPPRESS_OUT_OF_BOUND
+  // expected-warning@-2 {{UNKNOWN}}
+#endif
+  free(privkey);
+}
+
 //===--===
 // FIXMEs
 //===--===
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -124,6 +124,7 @@
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
   void evalMemset(CheckerContext , const CallExpr *CE) const;
+  void evalBzero(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -158,7 +159,7 @@
   static bool SummarizeRegion(raw_ostream , ASTContext ,
   const MemRegion *MR);
 
-  static bool memsetAux(const Expr *DstBuffer, const Expr *CharE,
+  static bool memsetAux(const Expr *DstBuffer, SVal CharE,
 const Expr *Size, CheckerContext ,
 ProgramStateRef );
 
@@ -1005,11 +1006,10 @@
   }
 }
 
-bool CStringChecker::memsetAux(const Expr *DstBuffer, const Expr *CharE,
+bool CStringChecker::memsetAux(const Expr *DstBuffer, SVal CharVal,
const Expr *Size, CheckerContext ,
ProgramStateRef ) {
   SVal MemVal = C.getSVal(DstBuffer);
-  SVal CharVal = C.getSVal(CharE);
   SVal SizeVal = C.getSVal(Size);
   const MemRegion *MR = MemVal.getAsRegion();
   if (!MR)
@@ -2184,13 +2184,59 @@
   // According to the values of the arguments, bind the value of the second
   // argument to the destination buffer and set string length, or just
   // invalidate the destination buffer.
-  if (!memsetAux(Mem, CharE, Size, C, State))
+  if (!memsetAux(Mem, C.getSVal(CharE), Size, C, State))
 return;
 
   State = State->BindExpr(CE, LCtx, MemVal);
   C.addTransition(State);
 }
 
+void CStringChecker::evalBzero(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() != 2)
+return;
+
+  CurrentFunctionDescription = "memory clearance function";
+
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Size = CE->getArg(1);
+  SVal Zero = C.getSValBuilder().makeZeroVal(C.getASTContext().IntTy);
+
+  ProgramStateRef State = C.getState();
+  
+  // See if the size argument is zero.
+  SVal SizeVal = C.getSVal(Size);
+  QualType SizeTy = Size->getType();
+
+  ProgramStateRef StateZeroSize, StateNonZeroSize;
+  std::tie(StateZeroSize, StateNonZeroSize) =
+assumeZero(C, 

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[David CARLIER via Phabricator via cfe-commits]

devnexen marked an inline comment as done.
devnexen added inline comments.



Comment at: test/Analysis/string.c:1405-1406
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str + 2, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{FALSE}}
+}

NoQ wrote:
> Let's also add the true statement. I.e., do we know here that the actual 
> length is 2?
I think that s the limit of this checker (even with memset that does not work).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:127
   void evalMemset(CheckerContext , const CallExpr *CE) const;
+  void evalExplicitBzero(CheckerContext , const CallExpr *CE) const;
 

I guess let's do just `evalBzero`?



Comment at: test/Analysis/string.c:1405-1406
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str + 2, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{FALSE}}
+}

Let's also add the true statement. I.e., do we know here that the actual length 
is 2?



Comment at: test/Analysis/string.c:1424
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void explicit_bzero3_out_ofbound() {

You can hide `exected-warning`s under `#ifdef`s while leaving the rest of the 
test intact. Eg.,

```
x = 0;
y = 1 / x;
#ifdef CHECK_DIVIDE_ZERO
// expected-warning@-2{{Division by zero}}
#endif
```

It would be cool to see how this test performs without suppressing out-of-bound 
warnings.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[David CARLIER via Phabricator via cfe-commits]

devnexen added a comment.

In D54592#1319421 , @Szelethus wrote:

> I hope you don't mind me changing the revision title -- many of us are 
> automatically subscribed to revisions with `analyzer` in the title, that also 
> helps with getting feedback sooner :)


Not at all ;-)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[David CARLIER via Phabricator via cfe-commits]

devnexen updated this revision to Diff 176780.
devnexen added a comment.

- Using same type for zero as memset.
- Updating slighty the unit tests with ptr/dynamic array for bzero.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1184,11 +1184,14 @@
 }
 
 //===--===
-// memset()
+// memset() / explicit_bzero() / bzero()
 //===--===
 
 void *memset(void *dest, int ch, size_t count);
 
+void bzero(void *dst, size_t count);
+void explicit_bzero(void *dest, size_t count);
+
 void *malloc(size_t size);
 void free(void *);
 
@@ -1383,6 +1386,52 @@
   clang_analyzer_eval(array[4] == 0); // expected-warning{{TRUE}}
 }
 
+void bzero1_null() {
+  char *a = NULL;
+
+  bzero(a, 10); // expected-warning{{Null pointer argument in call to memory clearance function}}
+}
+
+void bzero2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void bzero3_char_ptr_null() {
+  char *str = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  bzero(str + 2, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{FALSE}}
+}
+
+void explicit_bzero1_null() {
+  char *a = NULL;
+
+  explicit_bzero(a, 10); // expected-warning{{Null pointer argument in call to memory clearance function}}
+}
+
+void explicit_bzero2_clear_mypassword() {
+  char passwd[7] = "passwd";
+
+  explicit_bzero(passwd, sizeof(passwd)); // no-warning
+
+  clang_analyzer_eval(strlen(passwd) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(passwd[0] == '\0'); // expected-warning{{TRUE}}
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void explicit_bzero3_out_ofbound() {
+  char *privkey = (char *)malloc(6);
+  const char newprivkey[10] = "mysafekey";
+
+  strcpy(privkey, "random");
+  explicit_bzero(privkey, sizeof(newprivkey));
+  clang_analyzer_eval(privkey[0] == '\0'); // expected-warning{{UNKNOWN}}
+  free(privkey);
+}
+#endif
 //===--===
 // FIXMEs
 //===--===
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -124,6 +124,7 @@
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
   void evalMemset(CheckerContext , const CallExpr *CE) const;
+  void evalExplicitBzero(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -158,7 +159,7 @@
   static bool SummarizeRegion(raw_ostream , ASTContext ,
   const MemRegion *MR);
 
-  static bool memsetAux(const Expr *DstBuffer, const Expr *CharE,
+  static bool memsetAux(const Expr *DstBuffer, SVal CharE,
 const Expr *Size, CheckerContext ,
 ProgramStateRef );
 
@@ -1005,11 +1006,10 @@
   }
 }
 
-bool CStringChecker::memsetAux(const Expr *DstBuffer, const Expr *CharE,
+bool CStringChecker::memsetAux(const Expr *DstBuffer, SVal CharVal,
const Expr *Size, CheckerContext ,
ProgramStateRef ) {
   SVal MemVal = C.getSVal(DstBuffer);
-  SVal CharVal = C.getSVal(CharE);
   SVal SizeVal = C.getSVal(Size);
   const MemRegion *MR = MemVal.getAsRegion();
   if (!MR)
@@ -2184,13 +2184,57 @@
   // According to the values of the arguments, bind the value of the second
   // argument to the destination buffer and set string length, or just
   // invalidate the destination buffer.
-  if (!memsetAux(Mem, CharE, Size, C, State))
+  if (!memsetAux(Mem, C.getSVal(CharE), Size, C, State))
 return;
 
   State = State->BindExpr(CE, LCtx, MemVal);
   C.addTransition(State);
 }
 
+void CStringChecker::evalExplicitBzero(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() != 2)
+return;
+
+  CurrentFunctionDescription = "memory clearance function";
+
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Size = CE->getArg(1);
+  SVal Zero = C.getSValBuilder().makeZeroVal(C.getASTContext().IntTy);
+
+  ProgramStateRef State = C.getState();
+  
+  // See if the size argument is zero.
+  SVal SizeVal = C.getSVal(Size);
+  QualType SizeTy = Size->getType();
+
+  ProgramStateRef StateZeroSize, StateNonZeroSize;
+  std::tie(StateZeroSize, StateNonZeroSize) =
+

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[David CARLIER via Phabricator via cfe-commits]

devnexen marked an inline comment as done.
devnexen added inline comments.



Comment at: test/Analysis/string.c:1399
+  bzero(str, 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{UNKNOWN}}
+}

NoQ wrote:
> I suspect that the reason why this didn't work is that you forgot 
> `.addTransition()`. 
> 
> Could you also test that `bzero(str + 2, 2);` doesn't turn `strlen(str)` into 
> zero?
Good point(s).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54592: [analyzer][CStringChecker] evaluate explicit_bzero

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I hope you don't mind me changing the revision title -- many of us are 
automatically subscribed to revisions with `analyzer` in the title, that also 
helps with getting feedback sooner :)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54592/new/

https://reviews.llvm.org/D54592



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits