[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D31868#904814, @MTC wrote:

> > One of the possible improvements for future work here would be to actually 
> > bind the second argument value to the buffer instead of just invalidating 
> > it. Like, after `memset(buf, 0, sizeof(buf))` the analyzer should know that 
> > all values in the `buf` array are `0`. In the analyzer we have the notion 
> > of *default bindings* to handle that (see documentation in 
> > docs/analyzer/RegionStore.txt for more details).
>
> `BindDefault()` is the only function that can make the default binding, is 
> it? If so, `evalMemset()` uses `bindDefault()`, the binding may not take 
> effect. Because the current `BindDefault()` logic is that if the memory area 
> has been initialized, then the default binding will no longer be done, see 
> https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/RegionStore.cpp#L429.
>  Before `evalMemset()`, `MallocMemAux()` in MallocChecker.cpp may have 
> already made the default binding. Am I right?


Seems so, and it also looks super counter-intuitive. We definitely do need to 
overwrite default bindings from time to time. When `RegionStore` itself wants 
to overwrite a default binding, it does `removeBindings()` first (see 
`invalidateGlobalRegion()` as an example). I think 
`ProgramState::bindDefault()` should behave this way as well, not sure about 
impact of such change.


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Dear Henry,

Sorry for my poor English, please Create a New patch or provide Chinese 
translation :)

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

> One of the possible improvements for future work here would be to actually 
> bind the second argument value to the buffer instead of just invalidating it. 
> Like, after `memset(buf, 0, sizeof(buf))` the analyzer should know that all 
> values in the `buf` array are `0`. In the analyzer we have the notion of 
> *default bindings* to handle that (see documentation in 
> docs/analyzer/RegionStore.txt for more details).

`BindDefault()` is the only function that can make the default binding, is it? 
If so, `evalMemset()` uses `bindDefault()`, the binding may not take effect. 
Because the current `BindDefault()` logic is that if the memory area has been 
initialized, then the default binding will no longer be done, see 
https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/RegionStore.cpp#L429.
 Before `evalMemset()`, `MallocMemAux()` in MallocChecker.cpp may have already 
made the default binding. Am I right?

Thank you!


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL305773: [analyzer] Check NULL pointer dereference issue for 
memset function (authored by xiangzhai).

Changed prior to commit:
  https://reviews.llvm.org/D31868?vs=101847=103161#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D31868

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  cfe/trunk/test/Analysis/null-deref-ps-region.c

Index: cfe/trunk/test/Analysis/null-deref-ps-region.c
===
--- cfe/trunk/test/Analysis/null-deref-ps-region.c
+++ cfe/trunk/test/Analysis/null-deref-ps-region.c
@@ -1,6 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -std=gnu99 -analyzer-store=region -verify %s
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,unix,alpha.unix -std=gnu99 -analyzer-store=region -verify %s
 
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset(void *__s, int __c, size_t __n);
+void *malloc(size_t __size);
+void free(void *__ptr);
 
 // The store for 'a[1]' should not be removed mistakenly. SymbolicRegions may
 // also be live roots.
@@ -13,3 +18,55 @@
 i = *p; // no-warning
   }
 }
+
+void foo() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, sizeof(int));
+  int n = 1 / *x; // FIXME: no-warning
+  free(x);
+}
+
+void bar() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, 1);
+  int n = 1 / *x; // no-warning
+  free(x);
+}
+
+void testConcreteNull() {
+  int *x = 0;
+  memset(x, 0, 1); // expected-warning {{Null pointer argument in call to memory set function}}
+}
+
+void testStackArray() {
+  char buf[13];
+  memset(buf, 0, 1); // no-warning
+}
+
+void testHeapSymbol() {
+  char *buf = (char *)malloc(13);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
+
+void testStackArrayOutOfBound() {
+  char buf[1];
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+}
+
+void testHeapSymbolOutOfBound() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+  free(buf);
+}
+
+void testStackArraySameSize() {
+  char buf[1];
+  memset(buf, 0, sizeof(buf)); // no-warning
+}
+
+void testHeapSymbolSameSize() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -120,6 +120,7 @@
   void evalStdCopy(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
+  void evalMemset(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -1999,6 +2000,54 @@
   C.addTransition(State);
 }
 
+void CStringChecker::evalMemset(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() != 3)
+return;
+
+  CurrentFunctionDescription = "memory set function";
+
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Size = CE->getArg(2);
+  ProgramStateRef State = C.getState();
+
+  // See if the size argument is zero.
+  const LocationContext *LCtx = C.getLocationContext();
+  SVal SizeVal = State->getSVal(Size, LCtx);
+  QualType SizeTy = Size->getType();
+
+  ProgramStateRef StateZeroSize, StateNonZeroSize;
+  std::tie(StateZeroSize, StateNonZeroSize) =
+assumeZero(C, State, SizeVal, SizeTy);
+
+  // Get the value of the memory area.
+  SVal MemVal = State->getSVal(Mem, LCtx);
+
+  // If the size is zero, there won't be any actual memory access, so
+  // just bind the return value to the Mem buffer and return.
+  if (StateZeroSize && !StateNonZeroSize) {
+StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, MemVal);
+C.addTransition(StateZeroSize);
+return;
+  }
+
+  // Ensure the memory area is not null.
+  // If it is NULL there will be a NULL pointer dereference.
+  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
+  if (!State)
+return;
+
+  State = CheckBufferAccess(C, State, Size, Mem);
+  if (!State)
+return;
+  State = InvalidateBuffer(C, State, Mem, C.getSVal(Mem),
+   /*IsSourceBuffer*/false, Size);
+  if (!State)
+return;
+
+  State = State->BindExpr(CE, LCtx, MemVal);
+  C.addTransition(State);
+}
+
 static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) {
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -2032,6 +2081,8 @@
 evalFunction =  ::evalMemcmp;
   else if (C.isCLibraryFunction(FDecl, "memmove"))
 evalFunction =  ::evalMemmove;
+  else if (C.isCLibraryFunction(FDecl, "memset"))
+evalFunction =  ::evalMemset;
   else if 

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Artem Dergachev via Phabricator via cfe-commits]

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

Thanks, this was very useful, please commit!


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Kindly ping Artem and Anna :)


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 101847.
xiangzhai added a comment.
Herald added a subscriber: xazax.hun.

Hi Artem,

I updated my patch please review it, thanks a lot!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/null-deref-ps-region.c

Index: test/Analysis/null-deref-ps-region.c
===
--- test/Analysis/null-deref-ps-region.c
+++ test/Analysis/null-deref-ps-region.c
@@ -1,6 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -std=gnu99 -analyzer-store=region -verify %s
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,unix,alpha.unix -std=gnu99 -analyzer-store=region -verify %s
 
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset(void *__s, int __c, size_t __n);
+void *malloc(size_t __size);
+void free(void *__ptr);
 
 // The store for 'a[1]' should not be removed mistakenly. SymbolicRegions may
 // also be live roots.
@@ -13,3 +18,55 @@
 i = *p; // no-warning
   }
 }
+
+void foo() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, sizeof(int));
+  int n = 1 / *x; // FIXME: no-warning
+  free(x);
+}
+
+void bar() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, 1);
+  int n = 1 / *x; // no-warning
+  free(x);
+}
+
+void testConcreteNull() {
+  int *x = 0;
+  memset(x, 0, 1); // expected-warning {{Null pointer argument in call to memory set function}}
+}
+
+void testStackArray() {
+  char buf[13];
+  memset(buf, 0, 1); // no-warning
+}
+
+void testHeapSymbol() {
+  char *buf = (char *)malloc(13);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
+
+void testStackArrayOutOfBound() {
+  char buf[1];
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+}
+
+void testHeapSymbolOutOfBound() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+  free(buf);
+}
+
+void testStackArraySameSize() {
+  char buf[1];
+  memset(buf, 0, sizeof(buf)); // no-warning
+}
+
+void testHeapSymbolSameSize() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -120,6 +120,7 @@
   void evalStdCopy(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
+  void evalMemset(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -1999,6 +2000,54 @@
   C.addTransition(State);
 }
 
+void CStringChecker::evalMemset(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() != 3)
+return;
+
+  CurrentFunctionDescription = "memory set function";
+
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Size = CE->getArg(2);
+  ProgramStateRef State = C.getState();
+
+  // See if the size argument is zero.
+  const LocationContext *LCtx = C.getLocationContext();
+  SVal SizeVal = State->getSVal(Size, LCtx);
+  QualType SizeTy = Size->getType();
+
+  ProgramStateRef StateZeroSize, StateNonZeroSize;
+  std::tie(StateZeroSize, StateNonZeroSize) =
+assumeZero(C, State, SizeVal, SizeTy);
+
+  // Get the value of the memory area.
+  SVal MemVal = State->getSVal(Mem, LCtx);
+
+  // If the size is zero, there won't be any actual memory access, so
+  // just bind the return value to the Mem buffer and return.
+  if (StateZeroSize && !StateNonZeroSize) {
+StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, MemVal);
+C.addTransition(StateZeroSize);
+return;
+  }
+
+  // Ensure the memory area is not null.
+  // If it is NULL there will be a NULL pointer dereference.
+  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
+  if (!State)
+return;
+
+  State = CheckBufferAccess(C, State, Size, Mem);
+  if (!State)
+return;
+  State = InvalidateBuffer(C, State, Mem, C.getSVal(Mem),
+   /*IsSourceBuffer*/false, Size);
+  if (!State)
+return;
+
+  State = State->BindExpr(CE, LCtx, MemVal);
+  C.addTransition(State);
+}
+
 static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) {
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -2032,6 +2081,8 @@
 evalFunction =  ::evalMemcmp;
   else if (C.isCLibraryFunction(FDecl, "memmove"))
 evalFunction =  ::evalMemmove;
+  else if (C.isCLibraryFunction(FDecl, "memset"))
+evalFunction =  ::evalMemset;
   else if (C.isCLibraryFunction(FDecl, "strcpy"))
 evalFunction =  ::evalStrcpy;
   else if (C.isCLibraryFunction(FDecl, "strncpy"))

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Hi Artem,

Long time no see! miss you :)

I will update my patch next Thursday! I am doing my work assignment about L4 
 right now.

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

The code looks good now! A few minor comments and we can commit this :)




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2010
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Const = CE->getArg(1);
+  const Expr *Size = CE->getArg(2);

This variable is unused. It might make buildbots angry.



Comment at: test/Analysis/null-deref-ps-region.c:25
+  memset(x, 0, sizeof(int));
+  int n = 1 / *x;
+  free(x);

Could you mark this as FIXME? Eg:
```
int n = 1 / *x; // FIXME: no-warning
```
Because eventually it should warn.



Comment at: test/Analysis/null-deref-ps-region.c:36
+
+void f531() {
+  int *x = 0;

Can we make function names more fancy?

Eg. "testConcreteNull", "testStackArray", "testHeapSymbol", etc.


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 97042.
xiangzhai added a comment.

Hi Artem,

Please review my updated patch, thanks a lot!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/null-deref-ps-region.c

Index: test/Analysis/null-deref-ps-region.c
===
--- test/Analysis/null-deref-ps-region.c
+++ test/Analysis/null-deref-ps-region.c
@@ -1,6 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -std=gnu99 -analyzer-store=region -verify %s
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,unix,alpha.unix -std=gnu99 -analyzer-store=region -verify %s
 
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset(void *__s, int __c, size_t __n);
+void *malloc(size_t __size);
+void free(void *__ptr);
 
 // The store for 'a[1]' should not be removed mistakenly. SymbolicRegions may
 // also be live roots.
@@ -13,3 +18,55 @@
 i = *p; // no-warning
   }
 }
+
+void foo() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, sizeof(int));
+  int n = 1 / *x;
+  free(x);
+}
+
+void bar() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, 1);
+  int n = 1 / *x; // no-warning
+  free(x);
+}
+
+void f531() {
+  int *x = 0;
+  memset(x, 0, 1); // expected-warning {{Null pointer argument in call to memory set function}}
+}
+
+void f61() {
+  char buf[13];
+  memset(buf, 0, 1); // no-warning
+}
+
+void f611() {
+  char *buf = (char *)malloc(13);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
+
+void f66() {
+  char buf[1];
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+}
+
+void f666() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+  free(buf);
+}
+
+void f77() {
+  char buf[1];
+  memset(buf, 0, sizeof(buf)); // no-warning
+}
+
+void f777() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -120,6 +120,7 @@
   void evalStdCopy(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
+  void evalMemset(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -1999,6 +2000,55 @@
   C.addTransition(State);
 }
 
+void CStringChecker::evalMemset(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() != 3)
+return;
+
+  CurrentFunctionDescription = "memory set function";
+
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Const = CE->getArg(1);
+  const Expr *Size = CE->getArg(2);
+  ProgramStateRef State = C.getState();
+
+  // See if the size argument is zero.
+  const LocationContext *LCtx = C.getLocationContext();
+  SVal SizeVal = State->getSVal(Size, LCtx);
+  QualType SizeTy = Size->getType();
+
+  ProgramStateRef StateZeroSize, StateNonZeroSize;
+  std::tie(StateZeroSize, StateNonZeroSize) =
+assumeZero(C, State, SizeVal, SizeTy);
+
+  // Get the value of the memory area.
+  SVal MemVal = State->getSVal(Mem, LCtx);
+
+  // If the size is zero, there won't be any actual memory access, so
+  // just bind the return value to the Mem buffer and return.
+  if (StateZeroSize && !StateNonZeroSize) {
+StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, MemVal);
+C.addTransition(StateZeroSize);
+return;
+  }
+
+  // Ensure the memory area is not null.
+  // If it is NULL there will be a NULL pointer dereference.
+  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
+  if (!State)
+return;
+
+  State = CheckBufferAccess(C, State, Size, Mem);
+  if (!State)
+return;
+  State = InvalidateBuffer(C, State, Mem, C.getSVal(Mem),
+   /*IsSourceBuffer*/false, Size);
+  if (!State)
+return;
+
+  State = State->BindExpr(CE, LCtx, MemVal);
+  C.addTransition(State);
+}
+
 static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) {
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -2032,6 +2082,8 @@
 evalFunction =  ::evalMemcmp;
   else if (C.isCLibraryFunction(FDecl, "memmove"))
 evalFunction =  ::evalMemmove;
+  else if (C.isCLibraryFunction(FDecl, "memset"))
+evalFunction =  ::evalMemset;
   else if (C.isCLibraryFunction(FDecl, "strcpy"))
 evalFunction =  ::evalStrcpy;
   else if (C.isCLibraryFunction(FDecl, "strncpy"))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Hi Artem,

Thank you so much! you are my mentor teach me patiently and carefully, I will 
update my patch tomorrow, good night from my timezone:)

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I've a feeling we need to roll this back a little bit.

The `memset()` function does the following:

1. Accesses pointers in range R = [first argument, first argument + third 
argument] and crashes when accessing an invalid pointer.
2. Writes second argument to all bytes in range R.
3. Returns its first argument.

Assuming R is an empty set, steps 1 and 2 are skipped.

We handle step 1 through `checkNonNull` and `checkBufferAccess`. These 
easy-to-use functions are already available in the checker, just pass the 
arguments there directly.

For step 2, we decided to skip most of the step, instead invalidating the whole 
//base region// around R. There's a separate task to come up with a better 
behavior here, but we decided to do it later, probably in the next patch. So 
for now, the relationship between the size of the buffer and the third argument 
are not relevant and should not be considered - this is an idea for the future.

Finally, step 3 can be done by binding the call expression to the symbolic 
value of the first argument through `BindExpr`. You are already doing this in 
the zero-size case, but it should be similar in all cases. There is no need to 
construct a new symbol here - just use the existing `SVal`.




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2034
+  // If the size can be nonzero, we have to check the other arguments.
+  if (stateNonZeroSize) {
+state = stateNonZeroSize;

danielmarjamaki wrote:
> use early return:
> 
>   if (!stateNonZeroSize)
> return;
In fact this early return is unnecessary. Of the two states returned by 
`assume()`, at least one is always non-null (we have an assertion for that). 
However, the since the situation `(StateZeroSize && !StateNonZeroSize)` was 
checked above, it follows from `(!StateNonZeroSize)` that `StateZeroSize` is 
also null, which contradicts the above.

So we can assert here that `StateNonZeroSize` is not null.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2045-2049
+  SVal RetVal = SB.getConjuredHeapSymbolVal(CE, LCtx, C.blockCount());
+  const SymbolicRegion *SR =
+  dyn_cast_or_null(RetVal.getAsRegion());
+  if (!SR)
+return;

Here you construct a new symbol that represents the pointer returned. However, 
we return our first argument, which is already denoted by symbolic value 
`MemVal`, so we don't need a new symbol here - we'd return `MemVal` directly, 
and work on it directly during invalidation.

Also, note that it's not necessarily on the heap.


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 96344.
xiangzhai added a comment.

Hi Artem,

Because `memcpy` checked NULL pointer dereference for `src`:

  state = checkNonNull(C, state, Source, srcVal);
  ...

so such testcase can not point out my fault:

  void f15 () { 
 
int *x = malloc(sizeof(int));   
 
memcpy(x, 0, sizeof(int)); // expected-warning {{Null pointer argument in 
call to memory copy function}}
int n = 1 / *x; 
 
free(x);
 
  }

And I have no idea how to copy `RetVal` to Mem `s`:

  if (StateSameSize) {  
  
SVal ConstVal = State->getSVal(Const, LCtx);
  
State = State->BindExpr(CE, LCtx, RetVal);  
  
// Actually bind the second argument value to the buffer.   
  
State = State->bindDefault(RetVal, ConstVal, LCtx); 
  
// FIXME: Copy to Mem   
  
const MemRegion *MR = RetVal.getAsRegion(); 
  
if (!MR)
  
  return;   
  
MR = MR->StripCasts();  
  
if (const TypedValueRegion *TVR = MR->getAs()) {  
  
  MemVal = SB.makeLazyCompoundVal(StoreRef(State->getStore(),   
  
  State->getStateManager().getStoreManager()), TVR);
  
  State = State->BindExpr(CE, LCtx, MemVal);
  
}   
  
C.addTransition(State); 
  
  }

Please give me some advice, thanks a lot!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/null-deref-ps-region.c

Index: test/Analysis/null-deref-ps-region.c
===
--- test/Analysis/null-deref-ps-region.c
+++ test/Analysis/null-deref-ps-region.c
@@ -1,6 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -std=gnu99 -analyzer-store=region -verify %s
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,unix,alpha.unix -std=gnu99 -analyzer-store=region -verify %s
 
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset(void *__s, int __c, size_t __n);
+void *malloc(size_t __size);
+void free(void *__ptr);
 
 // The store for 'a[1]' should not be removed mistakenly. SymbolicRegions may
 // also be live roots.
@@ -13,3 +18,69 @@
 i = *p; // no-warning
   }
 }
+
+void f15 () {
+  int *x = malloc(sizeof(int));
+  memcpy(x, 0, sizeof(int)); // expected-warning {{Null pointer argument in call to memory copy function}}
+  int n = 1 / *x;
+  free(x);
+}
+
+void foo() {
+  int *x = malloc(sizeof(int));
+  int *r = memset(x, 0, sizeof(int));
+  int n = 1 / *r; // expected-warning {{Division by zero}}
+  free(x);
+}
+
+void foo2() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, sizeof(int));
+  int n = 1 / *x;
+  free(x);
+}
+
+void bar() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, 1);
+  int n = 1 / *x; // no-warning
+  free(x);
+}
+
+void f531() {
+  int *x = 0;
+  memset(x, 0, 1); // expected-warning {{Null pointer argument in call to memory set function}}
+}
+
+void f61() {
+  char buf[13];
+  memset(buf, 0, 1); // no-warning
+}
+
+void f611() {
+  char *buf = (char *)malloc(13);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
+
+void f66() {
+  char buf[1];
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+}
+
+void f666() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+  free(buf);
+}
+
+void f77() {
+  char buf[1];
+  memset(buf, 0, sizeof(buf)); // no-warning
+}
+
+void f777() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -120,6 +120,7 @@
   void evalStdCopy(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void 

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 95690.
xiangzhai added a comment.

Hi Artem,

> so you're doing the binding thing now?

No! it only works for `RetVal` for example `int *ret = memset(x, 0, 
sizeof(int));`, please see my testcase:

  void foo() {
int *x = malloc(sizeof(int));
int *ret = memset(x, 0, sizeof(int));
int n = 1 / *ret; // expected-warning {{Division by zero}}
free(x);
  }

but not work for `MemVal` for example `int n = 1 / *x;`

Please carefully review my patch to point out my fault: wrongly use 
`bindDefault`? thanks a lot!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/null-deref-ps-region.c

Index: test/Analysis/null-deref-ps-region.c
===
--- test/Analysis/null-deref-ps-region.c
+++ test/Analysis/null-deref-ps-region.c
@@ -1,6 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -std=gnu99 -analyzer-store=region -verify %s
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,unix,alpha.unix -std=gnu99 -analyzer-store=region -verify %s
 
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset(void *__s, int __c, size_t __n);
+void *malloc(size_t __size);
+void free(void *__ptr);
 
 // The store for 'a[1]' should not be removed mistakenly. SymbolicRegions may
 // also be live roots.
@@ -13,3 +18,55 @@
 i = *p; // no-warning
   }
 }
+
+void foo() {
+  int *x = malloc(sizeof(int));
+  int *r = memset(x, 0, sizeof(int));
+  int n = 1 / *r; // expected-warning {{Division by zero}}
+  free(x);
+}
+
+void bar() {
+  int *x = malloc(sizeof(int));
+  memset(x, 0, 1);
+  int n = 1 / *x; // no-warning
+  free(x);
+}
+
+void f531() {
+  int *x = 0;
+  memset(x, 0, 1); // expected-warning {{Null pointer argument in call to memory set function}}
+}
+
+void f61() {
+  char buf[13];
+  memset(buf, 0, 1); // no-warning
+}
+
+void f611() {
+  char *buf = (char *)malloc(13);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
+
+void f66() {
+  char buf[1];
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+}
+
+void f666() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+  free(buf);
+}
+
+void f77() {
+  char buf[1];
+  memset(buf, 0, sizeof(buf)); // no-warning
+}
+
+void f777() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -120,6 +120,7 @@
   void evalStdCopy(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
+  void evalMemset(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -1999,6 +2000,79 @@
   C.addTransition(State);
 }
 
+void CStringChecker::evalMemset(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() != 3)
+return;
+
+  CurrentFunctionDescription = "memory set function";
+
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Const = CE->getArg(1);
+  const Expr *Size = CE->getArg(2);
+  ProgramStateRef State = C.getState();
+
+  // See if the size argument is zero.
+  const LocationContext *LCtx = C.getLocationContext();
+  SVal SizeVal = State->getSVal(Size, LCtx);
+  QualType SizeTy = Size->getType();
+
+  ProgramStateRef StateZeroSize, StateNonZeroSize;
+  std::tie(StateZeroSize, StateNonZeroSize) =
+assumeZero(C, State, SizeVal, SizeTy);
+
+  // Get the value of the memory area.
+  SVal MemVal = State->getSVal(Mem, LCtx);
+
+  // If the size is zero, there won't be any actual memory access, so
+  // just bind the return value to the Mem buffer and return.
+  if (StateZeroSize && !StateNonZeroSize) {
+StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, MemVal);
+C.addTransition(StateZeroSize);
+return;
+  }
+
+  if (!StateNonZeroSize)
+return;
+
+  // Ensure the memory area is not null.
+  // If it is NULL there will be a NULL pointer dereference.
+  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
+  if (!State)
+return;
+
+  SValBuilder  = C.getSValBuilder();
+  // Check the region's extent is equal to the Size parameter.
+  SVal RetVal = SB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
+  const SymbolicRegion *R =
+  dyn_cast_or_null(RetVal.getAsRegion());
+  if (!R)
+return;
+  if (Optional DefinedSize =
+  SizeVal.getAs()) {
+DefinedOrUnknownSVal Extent = R->getExtent(SB);
+ProgramStateRef StateSameSize, StateNotSameSize;
+

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Hi All,

Thanks for your review! I will update my patch tomorrow! Almost 4+ days no see, 
I miss you :)

Regards,
Leslie Zhaii


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Wow, so you're doing the binding thing now? Thanks! It was not critical for 
landing this patch, so you could have fixed comments here, allowing us to 
commit what's already done, and then proceed with further improvements. It's 
also easier to review and aligns with the LLVM's policy of incremental 
development.

Could you add test cases for the new feature? For instance,

  void foo() {
int *x = malloc(sizeof(int));
memset(x, 0, sizeof(int));
1 / *x; // expected-warning{{Division by zero}}
  }

  void bar() {
int *x = malloc(sizeof(int));
memset(x, 0, 1);
1 / *x; // no-warning
  }

Tests that involve setting memory to anything but 0 are also welcome!




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2066
+
+  if (StateSameSize) {
+SVal ConstVal = State->getSVal(Const, LCtx);

I believe that if the size is not the same, you'd still need to do invalidation.


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

Please click "Done" on fixed review comments.




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2038
+  // If the size can be nonzero, we have to check the other arguments.
+  if (StateNonZeroSize) {
+State = StateNonZeroSize;

This "if (StateNonZeroSize)" condition is useless now since you added a early 
return



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2039
+  if (StateNonZeroSize) {
+State = StateNonZeroSize;
+

I suggest code cleanup:
```
State = StateNonZeroSize;
State = checkNonNull(C, State, Mem, MemVal);
```
Change that to:
```
State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
```




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2063
+if (!State)
+return;
+  }

indentation. I personally like the clang-format-diff script also. That would 
cleanup all your changes.

cd llvm/tools/clang
svn diff | python tools/clang-format/clang-format-diff.py -i

It should work with git diff also but I have not tried that.


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Hi Artem,

If it looks good to you, I want to try commit by myself for testing commit, 
thanks!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 95273.
xiangzhai added a comment.

Hi All,

I have updated my patch as your suggestion, please review it, thanks a lot!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/null-deref-ps-region.c

Index: test/Analysis/null-deref-ps-region.c
===
--- test/Analysis/null-deref-ps-region.c
+++ test/Analysis/null-deref-ps-region.c
@@ -1,6 +1,11 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -std=gnu99 -analyzer-store=region -verify %s
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,unix,alpha.unix -std=gnu99 -analyzer-store=region -verify %s
 
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset(void *__s, int __c, size_t __n);
+void *malloc(size_t __size);
+void free(void *__ptr);
 
 // The store for 'a[1]' should not be removed mistakenly. SymbolicRegions may
 // also be live roots.
@@ -13,3 +18,41 @@
 i = *p; // no-warning
   }
 }
+
+void f531() {
+  int *x = 0;
+  memset(x, 0, 1); // expected-warning {{Null pointer argument in call to memory set function}}
+}
+
+void f61() {
+  char buf[13];
+  memset(buf, 0, 1); // no-warning
+}
+
+void f611() {
+  char *buf = (char *)malloc(13);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
+
+void f66() {
+  char buf[1];
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+}
+
+void f666() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+  free(buf);
+}
+
+void f77() {
+  char buf[1];
+  memset(buf, 0, sizeof(buf)); // no-warning
+}
+
+void f777() {
+  char *buf = (char *)malloc(1);
+  memset(buf, 0, 1); // no-warning
+  free(buf);
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -120,6 +120,7 @@
   void evalStdCopy(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
+  void evalMemset(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -1999,6 +2000,80 @@
   C.addTransition(State);
 }
 
+void CStringChecker::evalMemset(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() != 3)
+return;
+
+  CurrentFunctionDescription = "memory set function";
+
+  const Expr *Mem = CE->getArg(0);
+  const Expr *Const = CE->getArg(1);
+  const Expr *Size = CE->getArg(2);
+  ProgramStateRef State = C.getState();
+
+  // See if the size argument is zero.
+  const LocationContext *LCtx = C.getLocationContext();
+  SVal SizeVal = State->getSVal(Size, LCtx);
+  QualType SizeTy = Size->getType();
+
+  ProgramStateRef StateZeroSize, StateNonZeroSize;
+  std::tie(StateZeroSize, StateNonZeroSize) =
+assumeZero(C, State, SizeVal, SizeTy);
+
+  // Get the value of the memory area.
+  SVal MemVal = State->getSVal(Mem, LCtx);
+
+  // If the size is zero, there won't be any actual memory access, so
+  // just bind the return value to the Mem buffer and return.
+  if (StateZeroSize && !StateNonZeroSize) {
+StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, MemVal);
+C.addTransition(StateZeroSize);
+return;
+  }
+
+  if (!StateNonZeroSize)
+return;
+
+  // If the size can be nonzero, we have to check the other arguments.
+  if (StateNonZeroSize) {
+State = StateNonZeroSize;
+
+// Ensure the memory area is not null.
+// If it is NULL there will be a NULL pointer dereference.
+State = checkNonNull(C, State, Mem, MemVal);
+if (!State)
+  return;
+
+SValBuilder  = C.getSValBuilder();
+// Check the region's extent is equal to the Size parameter.
+SVal RetVal = SB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
+const SymbolicRegion *R =
+dyn_cast_or_null(RetVal.getAsRegion());
+if (!R)
+  return;
+if (Optional DefinedSize =
+SizeVal.getAs()) {
+  DefinedOrUnknownSVal Extent = R->getExtent(SB);
+  ProgramStateRef StateSameSize, StateNotSameSize;
+  std::tie(StateSameSize, StateNotSameSize) =
+  State->assume(SB.evalEQ(State, Extent, *DefinedSize));
+  if (StateNotSameSize) {
+State = CheckBufferAccess(C, State, Size, Mem);
+if (!State)
+return;
+  }
+
+  if (StateSameSize) {
+SVal ConstVal = State->getSVal(Const, LCtx);
+State = State->BindExpr(CE, LCtx, RetVal);
+// Actually bind the second argument value to the buffer.
+State = State->bindDefault(RetVal, ConstVal, LCtx);
+C.addTransition(State);
+  

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Hi Artem,

Thanks for your review! I will update my patch tomorrow :)

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This looks correct! Thanks for taking this up.

One of the possible improvements for future work here would be to actually bind 
the second argument value to the buffer instead of just invalidating it. Like, 
after `memset(buf, 0, sizeof(buf))` the analyzer should know that all values in 
the `buf` array are `0`. In the analyzer we have the notion of *default 
bindings* to handle that (see documentation in docs/analyzer/RegionStore.txt 
for more details). However, they would only work when the whole region is 
`memset`ed, not when we're wiping a smaller portion of the region. But the 
special case of `memset`ing the whole region is important enough to be worth 
handling separately anyway. In order to detect this special case, you can find 
the region's *extent* and see if it's exactly equal to the size argument 
(ideally, handle comparison with the `assume` method). This would work not only 
for arrays, but also for regions allocated via `malloc` or C++ operator `new`. 
There is an attempt to do a similar thing for `strdup` in 
https://reviews.llvm.org/D20863 (mixed with more complicated stuff).

Since we're in `CStringChecker`, we may also want to see how `memset` affects 
our model of string length. If we `memset` to zero everything from the 
beginning of the region, the length of the resulting C string is 0; if not to 
zero, then the length of the resulting C string is at least the size argument; 
if not from the beginning, then we've no idea, unless we knew the length before 
and it was less then the offset from which we started `memset`ing.




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2025
+
+  // If the size is zero, there won't be any actual memory access, so  
 
+  // just bind the return value to the destination buffer and return.

Trailing spaces here.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2037
+
+// Ensure the memory area pointed to by s is not null. 
+// If it is NULL there will be a NULL pointer dereference.

One more trailing space.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2051-2052
+
+// Invalidate the memory area pointed to by s (regular invalidation 
without 
+// pointer-escaping the address of the top-level region). 
+state = InvalidateBuffer(C, state, S, C.getSVal(S),

And a few more trailing spaces :) I usually catch those with a colored `git 
diff` and highlight them in my editor as well, and also have a hotkey to 
clang-format current line, which is probably the most handy.


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Hi Daniel,

Thanks for your review!

Sorry I am not available until this Friday, then I will update my patch.

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

Thanks! Looks like a valueable addition.




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2004
+void CStringChecker::evalMemset(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() < 3)
+return;

even better:  != 3



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2009
+
+  const Expr *S = CE->getArg(0);
+  const Expr *Size = CE->getArg(2);

The name "S" does not tell me much.. how about something like Data / DataArg / 
PtrArg / ..?



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2011
+  const Expr *Size = CE->getArg(2);
+  ProgramStateRef state = C.getState();
+

Variables should start with capital.. State, SizeVal, SizeTy, ...



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2034
+  // If the size can be nonzero, we have to check the other arguments.
+  if (stateNonZeroSize) {
+state = stateNonZeroSize;

use early return:

  if (!stateNonZeroSize)
return;


Repository:
  rL LLVM

https://reviews.llvm.org/D31868



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

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai created this revision.

Hi LLVM developers,

As Anna mentioned:

> One idea is to check that we do not pass a pointer that is known to be NULL 
> to functions that are known to dereference pointers such as memcpy. There is 
> a checker that determines if a null pointer could be dereferenced already, 
> but there is no extension to check if such a pointer could be passed to a 
> function tat could dereference it.

So I implemented `evalMemset` in the CStringChecker to detect null-deref issue. 
please review my patch, thanks a lot!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D31868

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/null-deref-ps-region.c

Index: test/Analysis/null-deref-ps-region.c
===
--- test/Analysis/null-deref-ps-region.c
+++ test/Analysis/null-deref-ps-region.c
@@ -1,6 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -std=gnu99 -analyzer-store=region -verify %s
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,unix,alpha.unix -std=gnu99 -analyzer-store=region -verify %s
 
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset (void *__s, int __c, size_t __n);
 
 // The store for 'a[1]' should not be removed mistakenly. SymbolicRegions may
 // also be live roots.
@@ -13,3 +16,18 @@
 i = *p; // no-warning
   }
 }
+
+void f531() {
+  int *x = 0;
+  memset(x, 0, 1); // expected-warning {{Null pointer argument in call to memory set function}}
+}
+
+void f66() {
+  char buf[1];
+  memset(buf, 0, 1024); // expected-warning {{Memory set function accesses out-of-bound array element}}
+}
+
+void f77() {
+  char buf[1];
+  memset(buf, 0, sizeof(buf)); // no-warning
+}
Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -120,6 +120,7 @@
   void evalStdCopy(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext , const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext , const CallExpr *CE) const;
+  void evalMemset(CheckerContext , const CallExpr *CE) const;
 
   // Utility methods
   std::pair
@@ -1999,6 +2000,63 @@
   C.addTransition(State);
 }
 
+void CStringChecker::evalMemset(CheckerContext , const CallExpr *CE) const {
+  if (CE->getNumArgs() < 3)
+return;
+
+  CurrentFunctionDescription = "memory set function";
+
+  const Expr *S = CE->getArg(0);
+  const Expr *Size = CE->getArg(2);
+  ProgramStateRef state = C.getState();
+
+  // See if the size argument is zero.
+  const LocationContext *LCtx = C.getLocationContext();
+  SVal sizeVal = state->getSVal(Size, LCtx);
+  QualType sizeTy = Size->getType();
+
+  ProgramStateRef stateZeroSize, stateNonZeroSize;
+  std::tie(stateZeroSize, stateNonZeroSize) =
+assumeZero(C, state, sizeVal, sizeTy);
+
+  // Get the value of the memory area pointed to by S.
+  SVal sVal = state->getSVal(S, LCtx);
+
+  // If the size is zero, there won't be any actual memory access, so   
+  // just bind the return value to the destination buffer and return.
+  if (stateZeroSize && !stateNonZeroSize) {
+stateZeroSize = stateZeroSize->BindExpr(CE, LCtx, sVal);
+C.addTransition(stateZeroSize);
+return;
+  }
+
+  // If the size can be nonzero, we have to check the other arguments.
+  if (stateNonZeroSize) {
+state = stateNonZeroSize;
+
+// Ensure the memory area pointed to by s is not null. 
+// If it is NULL there will be a NULL pointer dereference.
+state = checkNonNull(C, state, S, sVal);
+if (!state)
+  return;
+
+// Ensure the accesses are valid.
+state = CheckBufferAccess(C, state, Size, S);
+if (!state)
+  return;
+
+// Return the memory area pointed to by s buffer.
+state = state->BindExpr(CE, LCtx, sVal);
+
+// Invalidate the memory area pointed to by s (regular invalidation without 
+// pointer-escaping the address of the top-level region). 
+state = InvalidateBuffer(C, state, S, C.getSVal(S),
+ /*IsSourceBuffer*/false, Size);
+
+C.addTransition(state);
+  }
+}
+
 static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) {
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
@@ -2032,6 +2090,8 @@
 evalFunction =  ::evalMemcmp;
   else if (C.isCLibraryFunction(FDecl, "memmove"))
 evalFunction =  ::evalMemmove;
+  else if (C.isCLibraryFunction(FDecl, "memset"))
+evalFunction =  ::evalMemset;
   else if (C.isCLibraryFunction(FDecl, "strcpy"))
 evalFunction =  ::evalStrcpy;
   else if (C.isCLibraryFunction(FDecl, "strncpy"))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org