[PATCH] D85093: [analyzer] StdLibraryFunctionsChecker: Add support for new functions

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze created this revision.
zukatsinadze added reviewers: NoQ, Charusso, martong, Szelethus.
zukatsinadze added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
zukatsinadze requested review of this revision.

`toupper`, `tolower`, `toascii` functions were added to 
StdLibraryFunctionsChecker to fully cover CERT STR37-C rule:
https://wiki.sei.cmu.edu/confluence/x/BNcxBQ


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85093

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c

Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -45,7 +45,6 @@
   // bugpath-note{{TRUE}} \
   // bugpath-note{{Left side of '&&' is true}} \
   // bugpath-note{{'x' is <= 255}}
-
 }
 
 void test_alnum_symbolic2(int x) {
@@ -62,6 +61,114 @@
   }
 }
 
+int toupper(int);
+
+void test_toupper_concrete(int v) {
+  int ret = toupper(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_toupper_symbolic(int x) {
+  int ret = toupper(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+}
+
+void test_toupper_symbolic2(int x) {
+  if (x > 255) { // \
+// bugpath-note{{Assuming 'x' is > 255}} \
+// bugpath-note{{Taking true branch}}
+
+int ret = toupper(x); // \
+// report-warning{{Function argument constraint is not satisfied}} \
+// bugpath-warning{{Function argument constraint is not satisfied}} \
+// bugpath-note{{Function argument constraint is not satisfied}}
+
+(void)ret;
+  }
+}
+
+int tolower(int);
+
+void test_tolower_concrete(int v) {
+  int ret = tolower(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_tolower_symbolic(int x) {
+  int ret = tolower(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+}
+
+void test_tolower_symbolic2(int x) {
+  if (x > 255) { // \
+// bugpath-note{{Assuming 'x' is > 255}} \
+// bugpath-note{{Taking true branch}}
+
+int ret = tolower(x); // \
+// report-warning{{Function argument constraint is not satisfied}} \
+// bugpath-warning{{Function argument constraint is not satisfied}} \
+// bugpath-note{{Function argument constraint is not satisfied}}
+
+(void)ret;
+  }
+}
+
+int toascii(int);
+
+void test_toascii_concrete(int v) {
+  int ret = toascii(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_toascii_symbolic(int x) {
+  int ret = toascii(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+}
+
+void test_toascii_symbolic2(int x) {
+  if (x > 255) { // \
+// bugpath-note{{Assuming 'x' is > 255}} \
+// bugpath-note{{Taking true branch}}
+
+int ret = toascii(x); // \
+// report-warning{{Function argument constraint is not satisfied}} \
+// bugpath-warning{{Function argument constraint is not satisfied}} \
+// bugpath-note{{Function argument constraint is not satisfied}}
+
+(void)ret;
+  }
+}
+
 typedef struct FILE FILE;
 typedef typeof(sizeof(int)) size_t;
 size_t fread(void *restrict, size_t, size_t, FILE *restrict);
@@ -80,7 +187,7 @@
   // bugpath-note{{'buf' is not equal to null}}
 }
 void test_notnull_symbolic2(FILE *fp, int *buf) {
-  if (!buf) // bugpath-note{{Assuming 'buf' is null}} \
+  if (!buf)  // bugpath-note{{Assuming 'buf' is null}} \
 // bugpath-note{{Taking true branch}}
 fread(buf, sizeof(int), 10, fp); // \
 // report-warning{{Function argument constraint is not satisfied}} \
@@ -151,7 +258,7 @@
 }
 int __buf_size_arg_constrain

[PATCH] D85093: [analyzer] StdLibraryFunctionsChecker: Add support for new functions

[Zurab Tsinadze via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG25bbe234e4e7: [analyzer] StdLibraryFunctionsChecker: Add 
support for new functions (authored by zukatsinadze).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85093

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/test/Analysis/std-c-library-functions-arg-constraints.c

Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c
===
--- clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -45,7 +45,6 @@
   // bugpath-note{{TRUE}} \
   // bugpath-note{{Left side of '&&' is true}} \
   // bugpath-note{{'x' is <= 255}}
-
 }
 
 void test_alnum_symbolic2(int x) {
@@ -62,6 +61,114 @@
   }
 }
 
+int toupper(int);
+
+void test_toupper_concrete(int v) {
+  int ret = toupper(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_toupper_symbolic(int x) {
+  int ret = toupper(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+}
+
+void test_toupper_symbolic2(int x) {
+  if (x > 255) { // \
+// bugpath-note{{Assuming 'x' is > 255}} \
+// bugpath-note{{Taking true branch}}
+
+int ret = toupper(x); // \
+// report-warning{{Function argument constraint is not satisfied}} \
+// bugpath-warning{{Function argument constraint is not satisfied}} \
+// bugpath-note{{Function argument constraint is not satisfied}}
+
+(void)ret;
+  }
+}
+
+int tolower(int);
+
+void test_tolower_concrete(int v) {
+  int ret = tolower(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_tolower_symbolic(int x) {
+  int ret = tolower(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+}
+
+void test_tolower_symbolic2(int x) {
+  if (x > 255) { // \
+// bugpath-note{{Assuming 'x' is > 255}} \
+// bugpath-note{{Taking true branch}}
+
+int ret = tolower(x); // \
+// report-warning{{Function argument constraint is not satisfied}} \
+// bugpath-warning{{Function argument constraint is not satisfied}} \
+// bugpath-note{{Function argument constraint is not satisfied}}
+
+(void)ret;
+  }
+}
+
+int toascii(int);
+
+void test_toascii_concrete(int v) {
+  int ret = toascii(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_toascii_symbolic(int x) {
+  int ret = toascii(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+}
+
+void test_toascii_symbolic2(int x) {
+  if (x > 255) { // \
+// bugpath-note{{Assuming 'x' is > 255}} \
+// bugpath-note{{Taking true branch}}
+
+int ret = toascii(x); // \
+// report-warning{{Function argument constraint is not satisfied}} \
+// bugpath-warning{{Function argument constraint is not satisfied}} \
+// bugpath-note{{Function argument constraint is not satisfied}}
+
+(void)ret;
+  }
+}
+
 typedef struct FILE FILE;
 typedef typeof(sizeof(int)) size_t;
 size_t fread(void *restrict, size_t, size_t, FILE *restrict);
@@ -80,7 +187,7 @@
   // bugpath-note{{'buf' is not equal to null}}
 }
 void test_notnull_symbolic2(FILE *fp, int *buf) {
-  if (!buf) // bugpath-note{{Assuming 'buf' is null}} \
+  if (!buf)  // bugpath-note{{Assuming 'buf' is null}} \
 // bugpath-note{{Taking true branch}}
 fread(buf, sizeof(int), 10, fp); // \
 // report-warning{{Function argument constraint is not satisfied}} \
@@ -151,7 +258,7 @@
 }
 int __buf_size_arg_constraint_mul(const void *, size_t, size_t);
 void test_buf_size_concrete_with_multiplication() {
-  short buf[3]; // bugpath-note{{'buf' initialized here}}
+  short buf[3]; // bugpath-note{{'

[PATCH] D79358: [analyzer] CERT: STR37-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze created this revision.
zukatsinadze added reviewers: NoQ, Charusso, Szelethus.
zukatsinadze added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, dkrupp, 
donat.nagy, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun, 
mgorny.

This patch introduces a new checker:
`alpha.security.cert.str.37c`

This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/x/BNcxBQ
The check warns if the argument of a character handling 
function is not representable as unsigned char.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79358

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/Str37Checker.cpp
  clang/test/Analysis/cert/str37-c-fp-suppression.cpp
  clang/test/Analysis/cert/str37-c.cpp

Index: clang/test/Analysis/cert/str37-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str37-c.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.str.37c\
+// RUN:  -verify %s
+
+// Examples from the CERT rule's page.
+// https://wiki.sei.cmu.edu/confluence/x/BNcxBQ
+
+#include "../Inputs/system-header-simulator.h"
+
+int isspace(int c);
+
+size_t bad_usage(const char *s) {
+  const char *t = s;
+  size_t length = strlen(s) + 1;
+  while (isspace(*t) && (t - s < length)) {
+// expected-warning@-1 {{arguments to character-handling function `isspace` must be representable as an unsigned char}}
+++t;
+  }
+  return t - s;
+}
+
+size_t good_usage(const char *s) {
+  const char *t = s;
+  size_t length = strlen(s) + 1;
+  while (isspace((unsigned char)*t) && (t - s < length)) { // no warning
+++t;
+  }
+  return t - s;
+}
Index: clang/test/Analysis/cert/str37-c-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str37-c-fp-suppression.cpp
@@ -0,0 +1,200 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.str.37c\
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+
+int isspace(int c);
+int toupper(int c);
+int isalnum(int c);
+int isalpha(int c);
+int isascii(int c);
+int isblank(int c);
+int iscntrl(int c);
+int isdigit(int c);
+int isgraph(int c);
+int islower(int c);
+int isprint(int c);
+int ispunct(int c);
+int isupper(int c);
+int isxdigit(int c);
+int toascii(int c);
+int tolower(int c);
+
+namespace correct_use {
+
+int test_tolower(const char *c) {
+  char lowA = tolower('A'); // no warning
+  return tolower(97);   // no warning
+}
+
+int test_toascii(const char *c) {
+  return toascii((unsigned char)*c); // no warning
+}
+
+void test_toupper(const char *s) {
+  char c = 98;
+  c = toupper(c); // no warning
+  char b = -2;
+  b = toupper((unsigned char)b); // no warning
+}
+
+int test_isalnum() {
+  int a = 50;
+  int testEOF = isalnum(EOF); // no warning
+  return isalnum(a);  // no warning
+}
+
+int test_isalpha() {
+  char c = 'a';
+  int b = isalpha(255); // no warning
+  return isalpha(c);// no warning
+}
+
+int test_isascii() {
+  char c = 'a';
+  int b = isascii(200); // no warning
+  int A = isascii(65);  // no warning
+  return isascii(c);// no warning
+}
+
+int test_isblank() {
+  char c = ' ';
+  bool isEOFBlank = isblank(EOF); // no warning
+  return isblank(c);  // no warning
+}
+
+int test_iscntrl() {
+  char c = 2;
+  return iscntrl(c); // no warning
+}
+
+int test_isdigit() {
+  char c = '2';
+  return isdigit(c); // no warning
+}
+
+int test_isgraph() {
+  char c = '9';
+  return isgraph(c); // no warning
+}
+
+int test_islower() {
+  char c = 'a';
+  return islower(c); // no warning
+}
+
+int test_isprint(char c) {
+  return isprint((unsigned char)c); // no warning
+}
+
+int test_ispunct() {
+  char c = 'a';
+  char b = -2;
+  bool test_unsigned = ispunct((unsigned char)b); // no warning
+  bool not_true = ispunct(c); // no warning
+  return ispunct(2);  // no warning
+}
+
+int test_isupper(const char *b) {
+  char c = 'A';
+  bool A_is_upper = isupper(c);  // no warning
+  return isupper((unsigned char)*b); // no warning
+}
+
+int test_isxdigit() {
+  char hexa = '9';
+  char not_hexa = 'M';
+  return isxdigit(hexa) || isxdigit(not_hexa); // no warning
+}
+} // namespace correct_use
+
+namespace incorrect_use {
+
+int test_tolower() {
+  int a = 5;
+  int b = 7;
+  char c = a - b;
+  return tolower(c);
+  // expected-warning@-1 {{arguments to character-handling function `tolower` must be representable as an unsigned char}}
+}
+
+int test_toascii(const char *c) {
+  return toascii(*c);
+  // expected-warning@-1 {{arguments to character-handling function `toascii` must be representable as an unsigned char}}
+}
+
+void test_toupp

[PATCH] D79358: [analyzer] CERT: STR37-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

In D79358#2019755 , @Szelethus wrote:

> Adding @martong, because I fear that this is colliding with 
> StdLibraryFunctionsChecker. The warnings added here seem to be, in essence, 
> identical to D73898 .


Indeed. I think the best way forward is to add missing functions (I think these 
are `toascii`, `toupper`, `tolower`) to @martong's checker to guarantee that 
CERT rule is covered completely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79358



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

[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

@NoQ gentle ping.


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

https://reviews.llvm.org/D124244

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

[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze created this revision.
zukatsinadze added reviewers: NoQ, martong, steakhal, balazske, vsavchenko.
zukatsinadze added a project: clang.
Herald added subscribers: manas, ASDenysPetrov, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, 
mgorny.
Herald added a project: All.
zukatsinadze requested review of this revision.
Herald added a subscriber: cfe-commits.

The patch adds two new checkers. 
//core.StoreToImmutable// that warns binds on immutable memory and
//alpha.security.cert.env.ModelConstQualifiedReturn// that models return values
of some functions that should be considered const qualified. Based on SEI CERT 
ENV30-C: https://wiki.sei.cmu.edu/confluence/x/79UxBQ


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124244

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/cert/env30-c.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c

Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -32,6 +32,7 @@
 // CHECK-NEXT: core.NullDereference
 // CHECK-NEXT: core.StackAddrEscapeBase
 // CHECK-NEXT: core.StackAddressEscape
+// CHECK-NEXT: core.StoreToImmutable
 // CHECK-NEXT: core.UndefinedBinaryOperatorResult
 // CHECK-NEXT: core.VLASize
 // CHECK-NEXT: core.builtin.BuiltinFunctions
Index: clang/test/Analysis/cert/env30-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/env30-c.cpp
@@ -0,0 +1,216 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.StoreToImmutable,alpha.security.cert.env.ModelConstQualifiedReturn \
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+int setenv(const char *name, const char *value, int overwrite);
+void free(void *memblock);
+void *malloc(size_t size);
+char *strdup(const char *s);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char *, const char *);
+void non_const_parameter_function(char *e);
+void const_parameter_function(const char *);
+
+
+
+void getenv_test1() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{return value of 'getenv' should be treated as const-qualified}}
+  *p = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void getenv_test2() {
+  char *p;
+
+  p = getenv("VAR");
+  non_const_parameter_function(p); // FIXME: Maybe we should warn for these.
+}
+
+void getenv_test3() {
+  char *p;
+  p = getenv("VAR");
+  const_parameter_function(p); // no-warning
+}
+
+void modify(char *p) {
+  *p = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void getenv_test4() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{return value of 'getenv' should be treated as const-qualified}}
+  char *pp = p;
+  modify(pp); // Writing to immutable region within the call.
+  // expected-note@-1{{Calling 'modify'}}
+}
+
+void does_not_modify(char *p) {
+  *p;
+}
+
+void getenv_test5() {
+  char *p = getenv("VAR");
+  does_not_modify(p); // no-warning
+}
+
+void getenv_test6() {
+  static char *array[] = {0};
+
+  if (!array[0]) {
+// expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{return value of 'getenv' should be treated as const-qualified}}
+  }
+
+  *array[0] = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void getenv_test7() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{return value of 'getenv' should be treated as const-qualified}}
+  if (!p)
+// expected-note@-1{{Assuming 'p' is non-null}}
+// expected-note@-2{{Taking false branch}}
+return;
+  p[0] = '\0';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void setlocale_test() {
+  char *p = setlocale(0, "VAR");
+  // expected-note@-1{{return value of 'setlocale' should be treated as const-qualified}}
+  *p = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToIm

[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 424493.
zukatsinadze added a comment.

fix clang format


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

https://reviews.llvm.org/D124244

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/cert/env30-c.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c

Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -32,6 +32,7 @@
 // CHECK-NEXT: core.NullDereference
 // CHECK-NEXT: core.StackAddrEscapeBase
 // CHECK-NEXT: core.StackAddressEscape
+// CHECK-NEXT: core.StoreToImmutable
 // CHECK-NEXT: core.UndefinedBinaryOperatorResult
 // CHECK-NEXT: core.VLASize
 // CHECK-NEXT: core.builtin.BuiltinFunctions
Index: clang/test/Analysis/cert/env30-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/env30-c.cpp
@@ -0,0 +1,213 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.StoreToImmutable,alpha.security.cert.env.ModelConstQualifiedReturn \
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+int setenv(const char *name, const char *value, int overwrite);
+void free(void *memblock);
+void *malloc(size_t size);
+char *strdup(const char *s);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char *, const char *);
+void non_const_parameter_function(char *e);
+void const_parameter_function(const char *);
+
+void getenv_test1() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{return value of 'getenv' should be treated as const-qualified}}
+  *p = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void getenv_test2() {
+  char *p;
+
+  p = getenv("VAR");
+  non_const_parameter_function(p); // FIXME: Maybe we should warn for these.
+}
+
+void getenv_test3() {
+  char *p;
+  p = getenv("VAR");
+  const_parameter_function(p); // no-warning
+}
+
+void modify(char *p) {
+  *p = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void getenv_test4() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{return value of 'getenv' should be treated as const-qualified}}
+  char *pp = p;
+  modify(pp); // Writing to immutable region within the call.
+  // expected-note@-1{{Calling 'modify'}}
+}
+
+void does_not_modify(char *p) {
+  *p;
+}
+
+void getenv_test5() {
+  char *p = getenv("VAR");
+  does_not_modify(p); // no-warning
+}
+
+void getenv_test6() {
+  static char *array[] = {0};
+
+  if (!array[0]) {
+// expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{return value of 'getenv' should be treated as const-qualified}}
+  }
+
+  *array[0] = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void getenv_test7() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{return value of 'getenv' should be treated as const-qualified}}
+  if (!p)
+// expected-note@-1{{Assuming 'p' is non-null}}
+// expected-note@-2{{Taking false branch}}
+return;
+  p[0] = '\0';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void setlocale_test() {
+  char *p = setlocale(0, "VAR");
+  // expected-note@-1{{return value of 'setlocale' should be treated as const-qualified}}
+  *p = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void strerror_test() {
+  char *p = strerror(0);
+  // expected-note@-1{{return value of 'strerror' should be treated as const-qualified}}
+  *p = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{modifying immutable memory}}
+}
+
+void asctime_test() {
+  const tm *t;
+  char *p = asctime(t);
+  // expected-note@-1{{return value of 'asctime' should be treated as const-qualified}}
+
+  *p = 'A';
+  // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}}
+  // expected-no

[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

In D124244#3588671 , @steakhal wrote:

> Sorry for my late reply.
>
> It feels like we have some serious obstacles.
> The `check::PostCall` handler wants to mark some memory region immutable. 
> Currently, the checker creates a new //symbolic// memregion, spawned into the 
> //immutable// memory space. After this it simply re-binds the return value.
> However, only `eval::Call` handler is supposed (**must**) to bind the return 
> value, so the current implementation cannot land.
>
> I played with the trait idea, which was this:
>
> 1. Introduce a new program state trait with the 
> `REGISTER_SET_WITH_PROGRAMSTATE(ImmutableRegions, const ento::MemRegion *)`. 
> One should not bind values to these regions. (*)
> 2. The `check::PostCall` would simply insert into this set.
> 3. The `StoreToImmutableChecker`, at the `check::Bind` handler, would verify 
> that the region is not contained by the set - otherwise emit a report...
> 4. Surface this new trait to be reachable by the `Core` infrastructure.
> 5. Refactor all the `Core` functions introducing or expecting 
> `MemRegionManager::getGlobalsRegion`s to insert the region in question into 
> this `ImmutableRegions` set associated with the current `State`, producing 
> some new `State`.
>
> The last point is the most critical. Now, by design, `MemRegionManager` does 
> not refer to the `ProgramState`, hence we don't have one that we could mutate 
> by inserting into the `ImmutableRegions` set. Passing a `ProgramStateRef` 
> along all the functions is a nightmare. Trust me, I've tried it.
> That being said, eradicating `GlobalImmutableSpaceRegion` seems to be 
> challenging.
>
> I've settled on using the custom trait, and the `GlobalImmutableSpaceRegion` 
> memspace kind to detect if the store (bind) should be allowed or not.
> F23475561: UsingTraitsForTrackingImmutability.patch 
> 
>
> WDYT @NoQ, how could we get rid of the `GlobalImmutableSpaceRegion`?
>
> ---
>
> (*): Originally I wanted a set of `const MemSpaceRegion *`, but it turns out 
> a default eval called function which returns a plain old mutable pointer is 
> of `SymRegion{conj{}}` at the `UnknownSpaceRegion`. And we probably don't 
> want to mark immutable the whole `UnknownSpaceRegion` xD.

@NoQ what do you think about steakhal's proposal?


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

https://reviews.llvm.org/D124244

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

[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 430204.
zukatsinadze marked 14 inline comments as done.

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

https://reviews.llvm.org/D124244

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/StoreToImmutable.h
  clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/cert/env30-c.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c

Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -32,6 +32,7 @@
 // CHECK-NEXT: core.NullDereference
 // CHECK-NEXT: core.StackAddrEscapeBase
 // CHECK-NEXT: core.StackAddressEscape
+// CHECK-NEXT: core.StoreToImmutable
 // CHECK-NEXT: core.UndefinedBinaryOperatorResult
 // CHECK-NEXT: core.VLASize
 // CHECK-NEXT: core.builtin.BuiltinFunctions
Index: clang/test/Analysis/cert/env30-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/env30-c.cpp
@@ -0,0 +1,222 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,alpha.security.cert.env.ModelConstQualifiedReturn \
+// RUN: -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+int setenv(const char *name, const char *value, int overwrite);
+void free(void *memblock);
+void *malloc(size_t size);
+char *strdup(const char *s);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char *, const char *);
+void const_parameter_function(const char *);
+
+
+
+void getenv_test1() {
+  char *a, *b, *c;
+  a = getenv("VAR");
+  // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+  // expected-note@-2{{Value assigned to 'a'}}
+  b = a;
+  // expected-note@-1{{The value of 'a' is assigned to 'b'}}
+  c = b;
+  // expected-note@-1{{The value of 'b' is assigned to 'c'}}
+  *c = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void getenv_test2() {
+  char *p;
+  p = getenv("VAR");
+  const_parameter_function(p); // no-warning
+}
+
+void modify(char *p) {
+  *p = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void getenv_test3() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+  // expected-note@-2{{'p' initialized here}}
+  char *pp = p;
+  // expected-note@-1{{'pp' initialized to the value of 'p'}}
+  modify(pp); // Writing to immutable region within the call.
+  // expected-note@-1{{Calling 'modify'}}
+  // expected-note@-2{{Passing value via 1st parameter 'p'}}
+}
+
+void does_not_modify(char *p) {
+  *p;
+}
+
+void getenv_test4() {
+  char *p = getenv("VAR");
+  does_not_modify(p); // no-warning
+}
+
+void getenv_test5() {
+  static char *array[] = {0};
+
+  if (!array[0]) {
+// expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+// expected-note@-2{{Assigning value}}
+  }
+
+  *array[0] = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void getenv_test6() {
+  char *p = getenv("VAR");
+  // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}}
+  if (!p)
+// expected-note@-1{{Assuming 'p' is non-null}}
+// expected-note@-2{{Taking false branch}}
+return;
+  p[0] = '\0';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void setlocale_test() {
+  char *p = setlocale(0, "VAR");
+  // expected-note@-1{{Return value of 'setlocale' should be treated as const-qualified}}
+  // expected-note@-2{{'p' initialized here}}
+  *p = 'A';
+  // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}}
+  // expected-note@-2{{Modifying immutable memory}}
+}
+
+void strerror_test() {
+char *p = strerror(0);
+// expected-note@-1{{Return value of 'strerror' should be treated as const-qualified}}
+// expected-note@-2{{'p' initialized here}}
+*p = 'A';
+// expected-warning@-1

[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:767-769
 assert(isa(sreg) || isa(sreg) ||
-   isa(sreg));
+   isa(sreg) ||
+   isa(sreg));

steakhal wrote:
> Please merge these into a single `isa()`.
error: macro "assert" passed 4 arguments, but takes just 1
`assert(isa(sreg));`

So I had to add extra ()-s around `isa`, I guess macro expands weirdly? 



Comment at: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt:68-70
   MIGChecker.cpp
+  cert/ModelConstQualifiedReturnChecker.cpp
   MoveChecker.cpp

steakhal wrote:
> Put this in the right alphabetical place.
I think it is in the right alphabetical place not considering cert/ (other 
certs are in similar order), but maybe I should remove the cert directory, what 
do you think? 
It was created by me a few years ago, but I don't see a point now anymore.



Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:27-29
+  BugType ImmutableMemoryBind{this,
+  "Immutable memory should not be overwritten",
+  categories::MemoryError};

steakhal wrote:
> You could expose a pointer to this by creating a `const BugType *getBugType() 
> const;` getter method, which could be used by other checkers.
I did something similar based on SmartPtrChecker.



Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:43
+
+  if (!isa(R->getMemorySpace()))
+return;

NoQ wrote:
> This check catches a lot more regions than the ones produced by the other 
> checker. In particular, it'll warn on all global constants. Did you intend 
> this to happen? You don't seem to have tests for that.
Could you give an example? I tried with the following snippet and it didn't 
give a warning

```
int a = 1, b = 1;
void foo(int *p) {
  a = 2;
  p[b++] = 3;
  int *c = &a;
  *c = 5;
}
```



Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:52
+  ImmutableMemoryBind, "modifying immutable memory", ErrorNode);
+  Report->markInteresting(R);
+  C.emitReport(std::move(Report));

NoQ wrote:
> I also recommend `trackExpressionValue` to make sure we have all 
> reassignments highlighted as the value gets bounced between pointer 
> variables. The user will need proof that the pointer actually points to const 
> memory.
What expression should I use for tracking? I tried to simply cast `S` to 
`Expr`, but it didn't really work. 
Then I came up with something ugly: cast `S` to `BinaryOperator` -> `getLHS` -> 
`getSubExpr` -> `ignoreImpCasts`.  


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

https://reviews.llvm.org/D124244

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

[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

In D124244#3493307 , @martong wrote:

>> The patch adds two new checkers.
>
> I don't see any technical dependencies between the two, so, please split it 
> into two independent patches.

StoreToImmutable does not depend on the modeling checker, but I am using the 
BugType pointer from StoreToImmutable in the modeling checker for Note 
diagnostics. 
Should it still be split and just not merge the second patch until the first 
one is committed?

In D124244#3493310 , @martong wrote:

> Also, could you please update the `Static Analyzer` section of 
> `clang/docs/ReleaseNotes.rst` with the new checkers and their description?

Sorry, I missed this comment. I will add release notes in the next round.


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

https://reviews.llvm.org/D124244

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

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.
Herald added a subscriber: manas.

@NoQ can you please have another look at this? I think it will be a useful 
checker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

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

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
zukatsinadze marked an inline comment as done.
Closed by commit rG811b1736d91b: [analyzer] Add InvalidPtrChecker (authored by 
zukatsinadze).

Changed prior to commit:
  https://reviews.llvm.org/D97699?vs=372237&id=376902#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
  clang/test/Analysis/cert/env31-c.c
  clang/test/Analysis/cert/env34-c-cert-examples.c
  clang/test/Analysis/cert/env34-c.c

Index: clang/test/Analysis/cert/env34-c.c
===
--- /dev/null
+++ clang/test/Analysis/cert/env34-c.c
@@ -0,0 +1,331 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr\
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+
+typedef struct {
+  char * field;
+} lconv;
+lconv *localeconv(void);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char*, const char*);
+extern void foo(char *e);
+extern char* bar();
+
+
+void getenv_test1() {
+  char *p;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  *p; // no-warning, getenv result was assigned to the same pointer
+}
+
+void getenv_test2() {
+  char *p, *p2;
+
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test3() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  p3 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test4() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+  p3 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test5() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  p2 = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p3 = getenv("VAR3");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test6() {
+  char *p, *p2;
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR3");
+  // expected-note@-1{{previous function call was here}}
+  // expected-note@-2{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+
+  *p2; // no-warning
+
+  p = getenv("VAR4");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p; // no-warning
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test7() {
+  char *p, *p2;
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  foo(p);
+  // expected-warning@-1{{use of invalidated pointer 'p' in a function call}}
+  // expected-note@-2{{use of invalidated pointer 'p' in a function call}}
+}
+
+void getenv_test8() {
+  static const char *array[] = {
+ 0,
+ 0,
+ "/var/tmp",
+ "/usr/tmp",
+ "/tmp",
+ "."
+  };
+
+  if( !array[0] )
+  // expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{previous function call was here}}
+
+  if( !array[1] )
+  // expected-note@-1{{Taking true branch}}
+array[1] = getenv("TMPDIR");
+// expected-note@

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 350656.
zukatsinadze marked 2 inline comments as done.
zukatsinadze added a comment.

@balazske Thanks for the comments!

Updated diff after suggested changes.


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

https://reviews.llvm.org/D97699

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
  clang/test/Analysis/cert/env31-c.c
  clang/test/Analysis/cert/env34-c-cert-examples.c
  clang/test/Analysis/cert/env34-c.c

Index: clang/test/Analysis/cert/env34-c.c
===
--- /dev/null
+++ clang/test/Analysis/cert/env34-c.c
@@ -0,0 +1,331 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr\
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+
+typedef struct {
+  char * field;
+} lconv;
+lconv *localeconv(void);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char*, const char*);
+extern void foo(char *e);
+extern char* bar();
+
+
+void getenv_test1() {
+  char *p;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  *p; // no-warning, getenv result was assigned to the same pointer
+}
+
+void getenv_test2() {
+  char *p, *p2;
+
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test3() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  p3 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test4() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+  p3 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test5() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  p2 = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p3 = getenv("VAR3");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test6() {
+  char *p, *p2;
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR3");
+  // expected-note@-1{{previous function call was here}}
+  // expected-note@-2{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+
+  *p2; // no-warning
+
+  p = getenv("VAR4");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p; // no-warning
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test7() {
+  char *p, *p2;
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  foo(p);
+  // expected-warning@-1{{use of invalidated pointer 'p' in a function call}}
+  // expected-note@-2{{use of invalidated pointer 'p' in a function call}}
+}
+
+void getenv_test8() {
+  static const char *array[] = {
+ 0,
+ 0,
+ "/var/tmp",
+ "/usr/tmp",
+ "/tmp",
+ "."
+  };
+
+  if( !array[0] )
+  // expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{previous function call was here}}
+
+  if( !array[1] )
+  // expected-note@-1{{Taking true branch}}
+array[1] = getenv("TMPDIR");
+// expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *array[0];
+  // expected-warning@-1{{dereferencing an 

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze marked an inline comment as done and an inline comment as not done.
zukatsinadze added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:947
+
+} // end "alpha.cert.env"
+

balazske wrote:
> I have multiple issues with the documentation texts but they look not final 
> anyway. And the package and checker names could be better. There are already 
> checkers for CERT rules that do not reside in the cert package, it is not 
> good to have some checkers in a `cert` package and some not. It is likely 
> that checkers will check for more rules or parts of the rules. Checker 
> aliases are a better solution for the cert checker names. (And now the `cert` 
> would contain checker with name not matching a rule: `InvalidPtr`.) The name 
> `InvalidPtr` sounds too general, even if in an `env˙ package.
Agreed. We can come back to wordsmithing later.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:227
+  if (const auto *Reg =
+  State->get(FunctionName.data())) {
+const auto *PrevReg = *Reg;

balazske wrote:
> balazske wrote:
> > `auto` is not to be used if the type is not clearly visible in the context. 
> > And this would be exactly `auto **` here. (But better is `const MemRegion 
> > **`.) Makes the later statement (assign to `PrevReg`) "messy" (and `auto` 
> > is bad there too).
> `.data` is unnecessary here?
`.data` seems to be necessary. 

`no known conversion from 'llvm::StringRef' to 'typename 
ProgramStateTrait::key_type' (aka 'const char *') for 
1st argument`



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:341
+  State = State->set(
+  reinterpret_cast(const_cast(EnvpReg)));
+  C.addTransition(State);

balazske wrote:
> Are these casts really needed?
Yes, as steakhal mentioned above, trait is specialized for void*


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

https://reviews.llvm.org/D97699

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

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze created this revision.
zukatsinadze added reviewers: NoQ, vsavchenko, Charusso, Szelethus, martong.
zukatsinadze added a project: clang.
Herald added subscribers: steakhal, ASDenysPetrov, ormris, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, 
mgorny.
zukatsinadze requested review of this revision.
Herald added a subscriber: cfe-commits.

This patch introduces a new checker: `alpha.security.cert.env.InvalidPtr`

Checker finds usage of invalidated pointers.

Based on the following SEI CERT Rules:
ENV31-C: https://wiki.sei.cmu.edu/confluence/x/8tYxBQ
ENV34-C: https://wiki.sei.cmu.edu/confluence/x/5NUxBQ


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97699

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
  clang/test/Analysis/cert/env31-c/_putenv_s.c
  clang/test/Analysis/cert/env31-c/_wputenv_s.c
  clang/test/Analysis/cert/env31-c/putenv.c
  clang/test/Analysis/cert/env31-c/setenv.c
  clang/test/Analysis/cert/env31-c/unsetenv.c
  clang/test/Analysis/cert/env34-c-cert-examples.c
  clang/test/Analysis/cert/env34-c.c

Index: clang/test/Analysis/cert/env34-c.c
===
--- /dev/null
+++ clang/test/Analysis/cert/env34-c.c
@@ -0,0 +1,241 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr\
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+
+typedef struct {
+  char * field;
+} lconv;
+lconv *localeconv(void);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char*, const char*);
+extern void foo(char *e);
+extern char* bar();
+
+
+void getenv_test1() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p3 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  p2 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test2() {
+  char *p, *p2;
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR3");
+  // expected-note@-1{{previous function call was here}}
+  // expected-note@-2{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+
+  *p2; // no-warning
+
+  p = getenv("VAR4");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p; // no-warning
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test3() {
+  char *p, *p2;
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  foo(p);
+  // expected-warning@-1{{use of invalidated pointer 'p' in a function call}}
+  // expected-note@-2{{use of invalidated pointer 'p' in a function call}}
+}
+
+void getenv_test4() {
+  static const char *array[] = {
+ 0,
+ 0,
+ "/var/tmp",
+ "/usr/tmp",
+ "/tmp",
+ "."
+  };
+
+  if( !array[0] )
+  // expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{previous function call was here}}
+
+  if( !array[1] )
+  // expected-note@-1{{Taking true branch}}
+array[1] = getenv("TMPDIR");
+// expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *array[0];
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test5() {
+  char *p, *p2;
+  p = getenv("something");
+  p = bar();
+  p2 = getenv("something");
+  *p; // no-warning
+}
+
+void getenv_test6() {
+  strcmp(getenv("VAR1"), getenv("VAR2"));
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+  // expected-note@-2{{previous function call was here}}
+  // expected-warning@-3{{use of invalidated pointer 'getenv("VAR1")' in a function call}}
+  // expected-note@-4{{use of invalidated pointer 'getenv("VAR1")' in a function call}}
+}
+
+void setlocale_test1() {
+  char *p, *p2;
+  p = setlocale(0, "VAR");
+  *p; // no-warning
+
+  p = setlocale(0, "VAR2");
+  // e

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

Please suggest which package to use for the checker. 
CERT rules are ENV, however, it deals with non-ENV functions as well.

Also, I am having a problem with `checkDeadSymbols`, it is similar to one 
xazax.hun faced here: http://reviews.llvm.org/D14203 (many many years ago)
`envp` memory region is marked dead too early in case of aliasing. Please check 
the snippets, the second one is problematic:

  int main(int argc, char **argv, char *envp[]) {
putenv((char*) "NAME=VALUE"); // envp invalidated
envp[0]; // gives error
  }
  
  int main(int argc, char **argv, char *envp[]) {
char **e = envp;
putenv((char*) "NAME=VALUE"); // envp invalidated
e[0]; // does not give error :(
  
// warnOnDeadSymbol reports 'envp' dead here
  } 
  
  int main(int argc, char **argv, char *envp[]) {
char **e = envp;
putenv((char*) "NAME=VALUE"); // envp invalidated
e[0]; // gives error again
  
/*
  use 'envp' somehow here
*/
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

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

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 327150.
zukatsinadze added a comment.

Fixed docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
  clang/test/Analysis/cert/env31-c/_putenv_s.c
  clang/test/Analysis/cert/env31-c/_wputenv_s.c
  clang/test/Analysis/cert/env31-c/putenv.c
  clang/test/Analysis/cert/env31-c/setenv.c
  clang/test/Analysis/cert/env31-c/unsetenv.c
  clang/test/Analysis/cert/env34-c-cert-examples.c
  clang/test/Analysis/cert/env34-c.c

Index: clang/test/Analysis/cert/env34-c.c
===
--- /dev/null
+++ clang/test/Analysis/cert/env34-c.c
@@ -0,0 +1,241 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr\
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+
+typedef struct {
+  char * field;
+} lconv;
+lconv *localeconv(void);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char*, const char*);
+extern void foo(char *e);
+extern char* bar();
+
+
+void getenv_test1() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p3 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  p2 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test2() {
+  char *p, *p2;
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR3");
+  // expected-note@-1{{previous function call was here}}
+  // expected-note@-2{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+
+  *p2; // no-warning
+
+  p = getenv("VAR4");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p; // no-warning
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test3() {
+  char *p, *p2;
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  foo(p);
+  // expected-warning@-1{{use of invalidated pointer 'p' in a function call}}
+  // expected-note@-2{{use of invalidated pointer 'p' in a function call}}
+}
+
+void getenv_test4() {
+  static const char *array[] = {
+ 0,
+ 0,
+ "/var/tmp",
+ "/usr/tmp",
+ "/tmp",
+ "."
+  };
+
+  if( !array[0] )
+  // expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{previous function call was here}}
+
+  if( !array[1] )
+  // expected-note@-1{{Taking true branch}}
+array[1] = getenv("TMPDIR");
+// expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *array[0];
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test5() {
+  char *p, *p2;
+  p = getenv("something");
+  p = bar();
+  p2 = getenv("something");
+  *p; // no-warning
+}
+
+void getenv_test6() {
+  strcmp(getenv("VAR1"), getenv("VAR2"));
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+  // expected-note@-2{{previous function call was here}}
+  // expected-warning@-3{{use of invalidated pointer 'getenv("VAR1")' in a function call}}
+  // expected-note@-4{{use of invalidated pointer 'getenv("VAR1")' in a function call}}
+}
+
+void setlocale_test1() {
+  char *p, *p2;
+  p = setlocale(0, "VAR");
+  *p; // no-warning
+
+  p = setlocale(0, "VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = setlocale(0, "VAR3");
+  // expected-note@-1{{'setlocale' call may invalidate the the result of the previous 'setlocale'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void setlocale_test2(int flag) {
+  char *p, *p2;
+  p = setlocale(0, "VAR");
+  *p; // no-warning
+
+  p = setlocale(0, "VAR2");
+  // expected-note@-1{{previous function 

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

Attaching results of CodeChecker run on some projects.F15697529: cc_results.zip 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

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

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 327173.
zukatsinadze added a comment.

Removed code repetition from the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
  clang/test/Analysis/cert/env31-c.c
  clang/test/Analysis/cert/env34-c-cert-examples.c
  clang/test/Analysis/cert/env34-c.c

Index: clang/test/Analysis/cert/env34-c.c
===
--- /dev/null
+++ clang/test/Analysis/cert/env34-c.c
@@ -0,0 +1,241 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr\
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+
+typedef struct {
+  char * field;
+} lconv;
+lconv *localeconv(void);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char*, const char*);
+extern void foo(char *e);
+extern char* bar();
+
+
+void getenv_test1() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p3 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  p2 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test2() {
+  char *p, *p2;
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR3");
+  // expected-note@-1{{previous function call was here}}
+  // expected-note@-2{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+
+  *p2; // no-warning
+
+  p = getenv("VAR4");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p; // no-warning
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test3() {
+  char *p, *p2;
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  foo(p);
+  // expected-warning@-1{{use of invalidated pointer 'p' in a function call}}
+  // expected-note@-2{{use of invalidated pointer 'p' in a function call}}
+}
+
+void getenv_test4() {
+  static const char *array[] = {
+ 0,
+ 0,
+ "/var/tmp",
+ "/usr/tmp",
+ "/tmp",
+ "."
+  };
+
+  if( !array[0] )
+  // expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{previous function call was here}}
+
+  if( !array[1] )
+  // expected-note@-1{{Taking true branch}}
+array[1] = getenv("TMPDIR");
+// expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *array[0];
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test5() {
+  char *p, *p2;
+  p = getenv("something");
+  p = bar();
+  p2 = getenv("something");
+  *p; // no-warning
+}
+
+void getenv_test6() {
+  strcmp(getenv("VAR1"), getenv("VAR2"));
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+  // expected-note@-2{{previous function call was here}}
+  // expected-warning@-3{{use of invalidated pointer 'getenv("VAR1")' in a function call}}
+  // expected-note@-4{{use of invalidated pointer 'getenv("VAR1")' in a function call}}
+}
+
+void setlocale_test1() {
+  char *p, *p2;
+  p = setlocale(0, "VAR");
+  *p; // no-warning
+
+  p = setlocale(0, "VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = setlocale(0, "VAR3");
+  // expected-note@-1{{'setlocale' call may invalidate the the result of the previous 'setlocale'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void setlocale_test2(int flag) {
+  char *p, *p2;
+  p = setlocale(0, "VAR");
+  *p; // no-warning
+
+  p = setlocale(0, "VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  if (flag) {
+// expected-note@-1{{Assuming 'flag' is not equal to 0}}
+// expected-note@-2{{Taking true branch}}
+

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

@NoQ, thanks for the comments.

In D97699#2601804 , @NoQ wrote:

> ... and whether flagged code is expected to be always invalid.

C standard says "may be overwritten", so I guess it's undefined behavior.

In D97699#2601804 , @NoQ wrote:

> ... I want you to explain what *exactly* does the checker checks ...

There are two problems that are checked. 
First: if we have 'envp' argument of main present and we modify the environment 
in some way (say setenv, putenv...), it "may" reallocate the whole environment 
memory, but 'envp' will still point to the old location. 
Second: if we have a pointer pointing to the result of 'getenv' (and some other 
non-reentrant functions, there are 5 of them implemented now, but can be easily 
extended to others, since checker is general enough) and we call 'getenv' again 
the previous result "may" be overwritten.
The idea of the checker is simple and somewhat comparable to MallocChecker or 
UseAfterFree (in a sense that 'free' invalidates region).  We have a map of 
previous function call results and after a subsequent call, we invalidate the 
previous region. And we warn on dereference of such pointer or if we have it as 
an argument to function call (to avoid some false negatives), which is not 
inlined.
I will add a description in a checker file.

And about `checkDeadSymbols`,  I get your point, but I am interested why 
checker has different behavior on these two examples:

  int main(int argc, char **argv, char *envp[]) {
putenv((char*) "NAME=VALUE"); // envp invalidated
envp[0]; // gives error
  }



  int main(int argc, char **argv, char *envp[]) {
char **e = envp;
putenv((char*) "NAME=VALUE"); // envp invalidated
e[0]; // does not give error
  } 

If I manually force keeping `envp` region in state when I check `if 
(!SymReaper.isLiveRegion(E)) {` , then it reports as expected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

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

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa54d81f59796: [analyzer] CERT: POS34-C (authored by 
zukatsinadze).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71433

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
  clang/test/Analysis/cert/pos34-c.cpp

Index: clang/test/Analysis/cert/pos34-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+// Examples from the CERT rule's page.
+// https://wiki.sei.cmu.edu/confluence/x/6NYxBQ
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int snprintf(char *str, size_t size, const char *format, ...);
+
+namespace test_auto_var_used_bad {
+
+int volatile_memory1(const char *var) {
+  char env[1024];
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+  // expected-warning@-1 {{The 'putenv' function should not be called with arguments that have automatic storage}}
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+int test_static(const char *var) {
+  static char env[1024];
+
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+}
+
+int test_heap_memory(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL) {
+return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+free(env);
+return -1;
+  }
+  if (oldenv != NULL) {
+free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+} // namespace test_auto_var_used_good
Index: clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int rand();
+
+namespace test_auto_var_used_good {
+
+extern char *ex;
+int test_extern() {
+  return putenv(ex); // no-warning: extern storage class.
+}
+
+void foo(void) {
+  char *buffer = (char *)"huttah!";
+  if (rand() % 2 == 0) {
+buffer = (char *)malloc(5);
+strcpy(buffer, "woot");
+  }
+  putenv(buffer);
+}
+
+void bar(void) {
+  char *buffer = (char *)malloc(5);
+  strcpy(buffer, "woot");
+
+  if (rand() % 2 == 0) {
+free(buffer);
+buffer = (char *)"blah blah blah";
+  }
+  putenv(buffer);
+}
+
+void baz() {
+  char env[] = "NAME=value";
+  // TODO: False Positive
+  putenv(env);
+  // expected-warning@-1 {{The 'putenv' function should not be called with arguments that have automatic storage}}
+
+  /*
+DO SOMETHING
+  */
+
+  putenv((char *)"NAME=anothervalue");
+}
+
+} // namespace test_auto_var_used_good
Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
===
--- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
+++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
@@ -9,13 +9,18 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 
 // Common strings used for the "category" of many static analyzer issues.
-namespace clang { namespace ento { namespace categories {
+namespace clang {
+namespace ento {
+namespace categories {
 
-const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
-const char * const LogicError = "Logic error";
-const char * const MemoryRefCount =
-  "Memory (Core Foundation/Objective-C/OSObject)";
-const char * const MemoryError = "Memory error";
-const char * const UnixAPI = "Unix API";
-const char * const CXXObjectLifecycle = "C++ object lifecycle";
-}}}
+const char *const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
+const char *const LogicError = "Logic error";
+const char *const MemoryRefCount =
+"Memory (Core Foundation/Objective-C/OSObject)";
+const char *const MemoryError = "Memory error";
+const

[PATCH] D70823: [clang-tidy] Adding cert-pos34-c check

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze created this revision.
zukatsinadze added reviewers: aaron.ballman, alexfh, hokein, Charusso.
zukatsinadze added projects: clang-tools-extra, clang.
Herald added subscribers: cfe-commits, mgehre, xazax.hun, mgorny.

According to
https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
cert-pos34-c check is created. The check warns if `putenv` function is 
called with automatic storage variable as an argument.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D70823

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.cpp
  clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-pos34-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/cert-pos34-c.cpp

Index: clang-tools-extra/test/clang-tidy/cert-pos34-c.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/cert-pos34-c.cpp
@@ -0,0 +1,64 @@
+// RUN: %check_clang_tidy %s cert-pos34-c %t
+#include 
+#include 
+
+namespace test_auto_var_used_bad {
+
+int func1(const char *var) {
+  char env[1024];
+  /*
+  Do Something
+  */
+  return putenv(env);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+int func2(char* a){
+  return putenv(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+
+void func3(char* a){
+  putenv(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+int func4(const char *var) {
+  static char env[1024];
+  /*
+  Do Something
+  */
+  return putenv(env); // no-warning: env is static.
+}
+
+// example from cert
+int func5(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *) malloc(len);
+  if (env == NULL) {
+return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+free(env);
+return -1;
+  }
+  if (oldenv != NULL) {
+free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+extern char* testExt;
+int func6(){
+  return putenv(testExt); // no-warning: extern storage class.
+}
+
+} // namespace test_auto_var_used_good
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -102,6 +102,7 @@
cert-msc51-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) 
+   cert-pos34-c
cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) 
cppcoreguidelines-avoid-goto
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
Index: clang-tools-extra/docs/clang-tidy/checks/cert-pos34-c.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cert-pos34-c.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - cert-pos34-c
+
+cert-pos34-c
+=
+
+Finds calls of ``putenv`` function with automatic variable as the argument.
+
+.. code-block:: c
+
+  #include 
+
+  int func(const char *var) {
+char env[1024];
+int retval = snprintf(env, sizeof(env),"TEST=%s", var);
+if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+}
+ 
+return putenv(env); // putenv function should not be called with auto variables
+  }
+
+
+This check corresponds to the CERT Standard rule 
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the argument.
+`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -194,6 +194,11 @@
   against self-assignment either by checking self-assignment explicitly or
   using the copy-and-swap or the copy-and-move method.
 
+- New :doc:`cert-pos34-c
+  ` check.
+
+  Finds calls of 'putenv' function with automatic variable as the argument.
+
 - New :doc:`fuchsia-default-arguments-calls
   ` check.
 
Index: clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.h

[PATCH] D70823: [clang-tidy] Adding cert-pos34-c check

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze marked 5 inline comments as done.
zukatsinadze added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.cpp:28
+  unless(hasDescendant(callExpr(callee(functionDecl(hasAnyName(
+  "::alloc", "::malloc", "::realloc", "::calloc")))
+  .bind("func"),

Charusso wrote:
> `alloc` -> `alloca`
I think ``alloca`` allocates memory on stack, so thats why I didn't include it 
here.


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

https://reviews.llvm.org/D70823



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

[PATCH] D70823: [clang-tidy] Adding cert-pos34-c check

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 231465.
zukatsinadze added a comment.

changes after review.


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

https://reviews.llvm.org/D70823

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.cpp
  clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-pos34-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/cert-pos34-c.cpp

Index: clang-tools-extra/test/clang-tidy/cert-pos34-c.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/cert-pos34-c.cpp
@@ -0,0 +1,63 @@
+// RUN: %check_clang_tidy %s cert-pos34-c %t
+#include 
+#include 
+
+namespace test_auto_var_used_bad {
+
+int func1(const char *var) {
+  char env[1024];
+  /*
+  Do Something
+  */
+  return putenv(env);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+int func2(char *a) {
+  return putenv(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+void func3(char *a) {
+  putenv(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'putenv' function should not be called with auto variables [cert-pos34-c]
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+int func4(const char *var) {
+  static char env[1024];
+  /*
+  Do Something
+  */
+  return putenv(env); // no-warning: env is static.
+}
+
+// example from cert
+int func5(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL) {
+return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+free(env);
+return -1;
+  }
+  if (oldenv != NULL) {
+free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+extern char *testExt;
+int func6() {
+  return putenv(testExt); // no-warning: extern storage class.
+}
+
+} // namespace test_auto_var_used_good
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -102,6 +102,7 @@
cert-msc51-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) 
+   cert-pos34-c
cppcoreguidelines-avoid-c-arrays (redirects to modernize-avoid-c-arrays) 
cppcoreguidelines-avoid-goto
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
Index: clang-tools-extra/docs/clang-tidy/checks/cert-pos34-c.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cert-pos34-c.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - cert-pos34-c
+
+cert-pos34-c
+=
+
+Finds calls of ``putenv`` function with automatic variable as the argument.
+
+.. code-block:: c
+
+  #include 
+
+  int func(const char *var) {
+char env[1024];
+int retval = snprintf(env, sizeof(env),"TEST=%s", var);
+if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+}
+ 
+return putenv(env); // putenv function should not be called with auto variables
+  }
+
+
+This check corresponds to the CERT Standard rule 
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the argument.
+`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -194,6 +194,11 @@
   against self-assignment either by checking self-assignment explicitly or
   using the copy-and-swap or the copy-and-move method.
 
+- New :doc:`cert-pos34-c
+  ` check.
+
+  Finds calls of ``putenv`` function with automatic variable as the argument.
+
 - New :doc:`fuchsia-default-arguments-calls
   ` check.
 
Index: clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.h
@@ -0,0 +1,34 @@
+//===--- PutenvWithAutoCheck.h - clang-tidy -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Ident

[PATCH] D70823: [clang-tidy] Adding cert-pos34-c check

[Zurab Tsinadze via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/cert/PutenvWithAutoCheck.cpp:27
+  hasAutomaticStorageDuration(),
+  unless(hasDescendant(callExpr(callee(functionDecl(hasAnyName(
+  "::alloc", "::malloc", "::realloc", "::calloc")))

aaron.ballman wrote:
> I don't know that this is sufficient for the check, and I sort of think this 
> may need to be implemented by the static analyzer rather than clang-tidy. The 
> initialization of the variable is going to be control flow sensitive. 
> Consider something like:
> ```
> void foo(void) {
>   char *buffer = "huttah!";
>   if (rand() % 2 == 0) {
> buffer = malloc(5);
> strcpy(buffer, "woot");
>   }
>   putenv(buffer);
> }
> 
> void bar(void) {
>   char *buffer = malloc(5);
>   strcpy(buffer, "woot");
> 
>   if (rand() % 2 == 0) {
> free(buffer);
> buffer = "blah blah blah";
>   }
>   putenv(buffer);
> }
> ```
Yes, I see your point. I will try to rewrite it as SA checker. 
Thanks for the review.


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

https://reviews.llvm.org/D70823



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

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze created this revision.
zukatsinadze added a reviewer: NoQ.
zukatsinadze added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun, mgorny.

This patch introduces a new checker:
`alpha.security.cert.pos.34c`

This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
The check warns if  `putenv ` function is
called with automatic storage variable as an argument.


Repository:
  rC Clang

https://reviews.llvm.org/D71433

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/test/Analysis/cert/pos34-c.cpp

Index: clang/test/Analysis/cert/pos34-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int snprintf(char *str, size_t size, const char *format, ...);
+
+namespace test_auto_var_used_bad {
+
+// example from cert
+int volatile_memory1(const char *var) {
+  char env[1024];
+  int retval = snprintf(env, sizeof(env),"TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+ 
+  return putenv(env);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+int volatile_memory2(char *a) {
+  return putenv(a);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+void volatile_memory3(char *a) {
+  char *buff = (char *)"hello";
+  putenv(buff);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+// example from cert
+int test_static(const char *var) {
+  static char env[1024];
+ 
+  int retval = snprintf(env, sizeof(env),"TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+ 
+  return putenv(env);
+}
+
+// example from cert
+int test_heap_memory(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL) {
+return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+free(env);
+return -1;
+  }
+  if (oldenv != NULL) {
+free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+extern char *ex;
+int test_extern() {
+  return putenv(ex); // no-warning: extern storage class.
+}
+
+} // namespace test_auto_var_used_good
Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
===
--- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
+++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
@@ -9,12 +9,17 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 
 // Common strings used for the "category" of many static analyzer issues.
-namespace clang { namespace ento { namespace categories {
+namespace clang {
+namespace ento {
+namespace categories {
 
-const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
-const char * const LogicError = "Logic error";
-const char * const MemoryRefCount =
-  "Memory (Core Foundation/Objective-C/OSObject)";
-const char * const MemoryError = "Memory error";
-const char * const UnixAPI = "Unix API";
-}}}
+const char *const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
+const char *const LogicError = "Logic error";
+const char *const MemoryRefCount =
+"Memory (Core Foundation/Objective-C/OSObject)";
+const char *const MemoryError = "Memory error";
+const char *const UnixAPI = "Unix API";
+const char *const SecurityError = "Security error";
+} // namespace categories
+} // namespace ento
+} // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
@@ -0,0 +1,68 @@
+//== PutenvWithAutoChecker.cpp - -*- C++ -*--=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 233946.
zukatsinadze added a comment.

Addressed most of the inline comments.


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

https://reviews.llvm.org/D71433

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
  clang/test/Analysis/cert/pos34-c.cpp

Index: clang/test/Analysis/cert/pos34-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+// Examples from the CERT rule's page.
+// https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int snprintf(char *str, size_t size, const char *format, ...);
+
+namespace test_auto_var_used_bad {
+
+int volatile_memory1(const char *var) {
+  char env[1024];
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+int test_static(const char *var) {
+  static char env[1024];
+
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+}
+
+int test_heap_memory(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL) {
+return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+free(env);
+return -1;
+  }
+  if (oldenv != NULL) {
+free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+} // namespace test_auto_var_used_good
Index: clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
@@ -0,0 +1,69 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int snprintf(char *str, size_t size, const char *format, ...);
+int rand();
+
+namespace test_auto_var_used_bad {
+
+int volatile_memory1(char *a) {
+  return putenv(a);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+void volatile_memory2(char *a) {
+  char *buff = (char *)"hello";
+  putenv(buff);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+void foo(void) {
+  char *buffer = (char *)"huttah!";
+  if (rand() % 2 == 0) {
+buffer = (char *)malloc(5);
+strcpy(buffer, "woot");
+  }
+  putenv(buffer);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+void bar(void) {
+  char *buffer = (char *)malloc(5);
+  strcpy(buffer, "woot");
+
+  if (rand() % 2 == 0) {
+free(buffer);
+buffer = (char *)"blah blah blah";
+  }
+  putenv(buffer);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+void baz() {
+  char env[] = "NAME=value";
+  // TODO: False Positive
+  putenv(env);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+
+  /*
+DO SOMETHING
+  */
+
+  putenv((char *)"NAME=anothervalue");
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+extern char *ex;
+int test_extern() {
+  return putenv(ex); // no-warning: extern storage class.
+}
+
+} // namespace test_auto_var_used_good
Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
===
--- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
+++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
@@ -9,13 +9,18 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 
 // Common strings used for the "category" of many static analyzer issues.
-namespace clang { namespace ento { namespace categori

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze marked 4 inline comments as done.
zukatsinadze added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:765
+  HelpText<"Finds calls to the `putenv` function which pass a pointer to "
+   "an automatic variable as the argument. (CERT POS 34C)">,
+  Documentation;

Charusso wrote:
> I would write ##`putenv`## -> `'putenv'` and the CERT rule-number should be 
> clear from that point so you could emit it.
Oops. Forgot this one. Will fix it later.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:45-52
+  if (const auto *DRE = dyn_cast(ArgExpr->IgnoreImpCasts()))
+if (const auto *VD = dyn_cast(DRE->getDecl()))
+  IsPossiblyAutoVar = isa(VD) && isa(MSR);
+
+  if (!IsPossiblyAutoVar &&
+  (isa(MSR) || isa(MSR) ||
+   isa(MSR) ||

NoQ wrote:
> Simply check whether the memory space is a stack memory space. This should be 
> a one-liner.
I could not get rid of `isPossiblyAutoVar` and `GlobalInternalSpaceRegion`. 



Comment at: clang/test/Analysis/cert/pos34-c-fp-suppression.cpp:15
+int volatile_memory1(char *a) {
+  return putenv(a);
+  // expected-warning@-1 {{'putenv' function should not be called with auto 
variables}}

I need `isPossiblyAutoVar` for this type. 



Comment at: clang/test/Analysis/cert/pos34-c-fp-suppression.cpp:20
+void volatile_memory2(char *a) {
+  char *buff = (char *)"hello";
+  putenv(buff);

And `GlobalInternalSpaceRegion` for this.


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

https://reviews.llvm.org/D71433



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

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

In D71433#1784238 , @NoQ wrote:

> Thanks! This looks like a simple and efficient check. I have one overall 
> suggestion.
>
> Currently the check may warn on non-bugs of the following kind:
>
>   void foo() {
> char env[] = "NAME=value";
> putenv(env);
> doStuff();
> putenv("NAME=anothervalue");
>   }
>
>
> I.e., it's obviously harmless if the local variable pointer is removed from 
> the environment before it goes out of scope. Can we instead warn when the 
> *last* `putenv()` on the execution path through the current stack frame is a 
> local variable (that goes out of scope at the end of the stack frame)?
>
> That'd allow the checker to be enabled by default, which will give a lot more 
> users access to it. Otherwise we'll have to treat it as an opt-in check and 
> users will only enable it when they know about it, which is much less 
> usefulness.


@NoQ I like the idea, but I am not really sure how to do that. I started 
working on Static Analyzer just lask week.


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

https://reviews.llvm.org/D71433



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

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 233955.
zukatsinadze added a comment.

- Removed extra test
- Used `CallDescription` for checking call.




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

https://reviews.llvm.org/D71433

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
  clang/test/Analysis/cert/pos34-c.cpp

Index: clang/test/Analysis/cert/pos34-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+// Examples from the CERT rule's page.
+// https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int snprintf(char *str, size_t size, const char *format, ...);
+
+namespace test_auto_var_used_bad {
+
+int volatile_memory1(const char *var) {
+  char env[1024];
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+int test_static(const char *var) {
+  static char env[1024];
+
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+}
+
+int test_heap_memory(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL) {
+return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+free(env);
+return -1;
+  }
+  if (oldenv != NULL) {
+free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+} // namespace test_auto_var_used_good
Index: clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int rand();
+
+namespace test_auto_var_used_good {
+
+extern char *ex;
+int test_extern() {
+  return putenv(ex); // no-warning: extern storage class.
+}
+
+void foo(void) {
+  char *buffer = (char *)"huttah!";
+  if (rand() % 2 == 0) {
+buffer = (char *)malloc(5);
+strcpy(buffer, "woot");
+  }
+  putenv(buffer);
+}
+
+void bar(void) {
+  char *buffer = (char *)malloc(5);
+  strcpy(buffer, "woot");
+
+  if (rand() % 2 == 0) {
+free(buffer);
+buffer = (char *)"blah blah blah";
+  }
+  putenv(buffer);
+}
+
+void baz() {
+  char env[] = "NAME=value";
+  // TODO: False Positive
+  putenv(env);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+
+  /*
+DO SOMETHING
+  */
+
+  putenv((char *)"NAME=anothervalue");
+}
+
+} // namespace test_auto_var_used_good
Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
===
--- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
+++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
@@ -9,13 +9,18 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 
 // Common strings used for the "category" of many static analyzer issues.
-namespace clang { namespace ento { namespace categories {
+namespace clang {
+namespace ento {
+namespace categories {
 
-const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
-const char * const LogicError = "Logic error";
-const char * const MemoryRefCount =
-  "Memory (Core Foundation/Objective-C/OSObject)";
-const char * const MemoryError = "Memory error";
-const char * const UnixAPI = "Unix API";
-const char * const CXXObjectLifecycle = "C++ object lifecycle";
-}}}
+const char *const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
+const char *const LogicError = "Logic error";
+const char *const MemoryRefCount =
+"Memory (

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 243127.
zukatsinadze added a comment.

Addressed new inline comments.


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

https://reviews.llvm.org/D71433

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
  clang/test/Analysis/cert/pos34-c.cpp

Index: clang/test/Analysis/cert/pos34-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+// Examples from the CERT rule's page.
+// https://wiki.sei.cmu.edu/confluence/x/6NYxBQ
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int snprintf(char *str, size_t size, const char *format, ...);
+
+namespace test_auto_var_used_bad {
+
+int volatile_memory1(const char *var) {
+  char env[1024];
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+int test_static(const char *var) {
+  static char env[1024];
+
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+}
+
+int test_heap_memory(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL) {
+return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+free(env);
+return -1;
+  }
+  if (oldenv != NULL) {
+free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+} // namespace test_auto_var_used_good
Index: clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int rand();
+
+namespace test_auto_var_used_good {
+
+extern char *ex;
+int test_extern() {
+  return putenv(ex); // no-warning: extern storage class.
+}
+
+void foo(void) {
+  char *buffer = (char *)"huttah!";
+  if (rand() % 2 == 0) {
+buffer = (char *)malloc(5);
+strcpy(buffer, "woot");
+  }
+  putenv(buffer);
+}
+
+void bar(void) {
+  char *buffer = (char *)malloc(5);
+  strcpy(buffer, "woot");
+
+  if (rand() % 2 == 0) {
+free(buffer);
+buffer = (char *)"blah blah blah";
+  }
+  putenv(buffer);
+}
+
+void baz() {
+  char env[] = "NAME=value";
+  // TODO: False Positive
+  putenv(env);
+  // expected-warning@-1 {{'putenv' function should not be called with auto variables}}
+
+  /*
+DO SOMETHING
+  */
+
+  putenv((char *)"NAME=anothervalue");
+}
+
+} // namespace test_auto_var_used_good
Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
===
--- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
+++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
@@ -9,13 +9,18 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 
 // Common strings used for the "category" of many static analyzer issues.
-namespace clang { namespace ento { namespace categories {
+namespace clang {
+namespace ento {
+namespace categories {
 
-const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
-const char * const LogicError = "Logic error";
-const char * const MemoryRefCount =
-  "Memory (Core Foundation/Objective-C/OSObject)";
-const char * const MemoryError = "Memory error";
-const char * const UnixAPI = "Unix API";
-const char * const CXXObjectLifecycle = "C++ object lifecycle";
-}}}
+const char *const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
+const char *const LogicError = "Logic error";
+const char *const MemoryRefCount =
+"Memory (Core Foundation/Objective-C/OSObject)";
+const char *const MemoryError = "Memory error";
+const char *const UnixAPI = "Unix API

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

In D71433#1863638 , @Charusso wrote:

> In D71433#1808316 , @Charusso wrote:
>
> > In D71433#1784238 , @NoQ wrote:
> >
> > > Currently the check may warn on non-bugs of the following kind:
> > >
> > >   void foo() {
> > > char env[] = "NAME=value";
> > > putenv(env);
> > > doStuff();
> > > putenv("NAME=anothervalue");
> > >   }
> > >
> >
> >
> > That could be the next round as a follow-up patch as the next semester 
> > starts in February [...]
>
>
> Well, the next semester is about to start. Could you implement that request 
> please?


Sure. I plan to start working on it next week.


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

https://reviews.llvm.org/D71433



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

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze marked 2 inline comments as done.
zukatsinadze added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:3383-3386
+std::unique_ptr getMallocBRVisitor(SymbolRef Sym) {
+  return std::make_unique(Sym);
+}
+

Szelethus wrote:
> And this?
Forgot to delete. Thanks


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

https://reviews.llvm.org/D71433



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

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 243149.
zukatsinadze marked an inline comment as done.
zukatsinadze added a comment.

- Removed dead code.
- Removed unnecessary if condition.
- Changed error phrasing.




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

https://reviews.llvm.org/D71433

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
  clang/test/Analysis/cert/pos34-c.cpp

Index: clang/test/Analysis/cert/pos34-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+// Examples from the CERT rule's page.
+// https://wiki.sei.cmu.edu/confluence/x/6NYxBQ
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int snprintf(char *str, size_t size, const char *format, ...);
+
+namespace test_auto_var_used_bad {
+
+int volatile_memory1(const char *var) {
+  char env[1024];
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+  // expected-warning@-1 {{The 'putenv' function should not be called with arguments that have automatic storage}}
+}
+
+} // namespace test_auto_var_used_bad
+
+namespace test_auto_var_used_good {
+
+int test_static(const char *var) {
+  static char env[1024];
+
+  int retval = snprintf(env, sizeof(env), "TEST=%s", var);
+  if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+  }
+
+  return putenv(env);
+}
+
+int test_heap_memory(const char *var) {
+  static char *oldenv;
+  const char *env_format = "TEST=%s";
+  const size_t len = strlen(var) + strlen(env_format);
+  char *env = (char *)malloc(len);
+  if (env == NULL) {
+return -1;
+  }
+  if (putenv(env) != 0) { // no-warning: env was dynamically allocated.
+free(env);
+return -1;
+  }
+  if (oldenv != NULL) {
+free(oldenv); /* avoid memory leak */
+  }
+  oldenv = env;
+  return 0;
+}
+
+} // namespace test_auto_var_used_good
Index: clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.pos.34c\
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+void free(void *memblock);
+void *malloc(size_t size);
+int putenv(char *);
+int rand();
+
+namespace test_auto_var_used_good {
+
+extern char *ex;
+int test_extern() {
+  return putenv(ex); // no-warning: extern storage class.
+}
+
+void foo(void) {
+  char *buffer = (char *)"huttah!";
+  if (rand() % 2 == 0) {
+buffer = (char *)malloc(5);
+strcpy(buffer, "woot");
+  }
+  putenv(buffer);
+}
+
+void bar(void) {
+  char *buffer = (char *)malloc(5);
+  strcpy(buffer, "woot");
+
+  if (rand() % 2 == 0) {
+free(buffer);
+buffer = (char *)"blah blah blah";
+  }
+  putenv(buffer);
+}
+
+void baz() {
+  char env[] = "NAME=value";
+  // TODO: False Positive
+  putenv(env);
+  // expected-warning@-1 {{The 'putenv' function should not be called with arguments that have automatic storage}}
+
+  /*
+DO SOMETHING
+  */
+
+  putenv((char *)"NAME=anothervalue");
+}
+
+} // namespace test_auto_var_used_good
Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
===
--- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
+++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
@@ -9,13 +9,18 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 
 // Common strings used for the "category" of many static analyzer issues.
-namespace clang { namespace ento { namespace categories {
+namespace clang {
+namespace ento {
+namespace categories {
 
-const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
-const char * const LogicError = "Logic error";
-const char * const MemoryRefCount =
-  "Memory (Core Foundation/Objective-C/OSObject)";
-const char * const MemoryError = "Memory error";
-const char * const UnixAPI = "Unix API";
-const char * const CXXObjectLifecycle = "C++ object lifecycle";
-}}}
+const char *const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
+const char *const LogicError = "Logic error";
+const char *const MemoryRefCount =
+"Memory (Core Foundation/Objective-C/OSObject)";
+const char *const MemoryError = "Memory

[PATCH] D71433: [analyzer] CERT: POS34-C

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added a comment.

In D71433#1880436 , @Szelethus wrote:

> I think for an alpha checker this is ready to land if you're ready -- do you 
> have commit access or need assistance?


Thank you. @Charusso will help.


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

https://reviews.llvm.org/D71433



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

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added inline comments.



Comment at: clang/docs/analyzer/checkers.rst:2103-2104
+puts(envp[i]);
+// envp may no longer point to the current environment
+// this program has unanticipated behavior.
+  }

Charusso wrote:
> NoQ wrote:
> > It's very important to explain whether the "unanticipated behavior" is an 
> > entirely psychological concept ("most people don't understand how this 
> > works but this can occasionally also be a valid solution if you know what 
> > you're doing") or a 100% clear indication of a bug ("such code can never be 
> > correct", eg. it instantly causes undefined behavior, or the written value 
> > can never be read later so there's absolutely no point in writing it, or 
> > something like that).
> The standard terminology is very vague, like that:
> ```
> The getenv function returns a pointer to a string associated with the matched 
> list member. The string pointed to shall not be modified by the program but 
> may be overwritten by a subsequent call to the getenv function.
> ```
> What the hell. 😕  I think it is about table-doubling and reallocating the 
> entire environment pointer table at some point which makes sense in case of 
> the non-getter function calls. For the getters I think another processes 
> could overwrite the `environ` pointer between two getter calls and problem 
> could occur because of such table-doubling. To resolve the issue we need to 
> call `strdup()` and create a copy of the current environment entry instead of 
> having a pointer to it as I see.
> 
> @zukatsinadze it would be great to see a real reference with real issues in 
> real world software. Is the true evil the multiple chained getter calls?
> 
> it would be great to see a real reference with real issues in real world 
> software

I've attached some results up in the thread. Checker gave several valuable 
reports on several projects if that's what you mean.

>  Is the true evil the multiple chained getter calls?

Besides SEI CERT, there are many other reputable resources stating the same 
problem with getenv.

https://www.ibm.com/support/knowledgecenter/SSLTBW_2.2.0/com.ibm.zos.v2r2.bpxbd00/getenv.htm
https://www.keil.com/support/man/docs/armlib/armlib_chr1359122850667.htm
https://pubs.opengroup.org/onlinepubs/009696799/functions/getenv.html
https://pubs.opengroup.org/onlinepubs/7908799/xsh/getenv.html
https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html
https://man7.org/linux/man-pages/man3/getenv.3p.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

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

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added inline comments.



Comment at: clang/docs/analyzer/checkers.rst:2103-2104
+puts(envp[i]);
+// envp may no longer point to the current environment
+// this program has unanticipated behavior.
+  }

zukatsinadze wrote:
> Charusso wrote:
> > NoQ wrote:
> > > It's very important to explain whether the "unanticipated behavior" is an 
> > > entirely psychological concept ("most people don't understand how this 
> > > works but this can occasionally also be a valid solution if you know what 
> > > you're doing") or a 100% clear indication of a bug ("such code can never 
> > > be correct", eg. it instantly causes undefined behavior, or the written 
> > > value can never be read later so there's absolutely no point in writing 
> > > it, or something like that).
> > The standard terminology is very vague, like that:
> > ```
> > The getenv function returns a pointer to a string associated with the 
> > matched list member. The string pointed to shall not be modified by the 
> > program but may be overwritten by a subsequent call to the getenv function.
> > ```
> > What the hell. 😕  I think it is about table-doubling and reallocating the 
> > entire environment pointer table at some point which makes sense in case of 
> > the non-getter function calls. For the getters I think another processes 
> > could overwrite the `environ` pointer between two getter calls and problem 
> > could occur because of such table-doubling. To resolve the issue we need to 
> > call `strdup()` and create a copy of the current environment entry instead 
> > of having a pointer to it as I see.
> > 
> > @zukatsinadze it would be great to see a real reference with real issues in 
> > real world software. Is the true evil the multiple chained getter calls?
> > 
> > it would be great to see a real reference with real issues in real world 
> > software
> 
> I've attached some results up in the thread. Checker gave several valuable 
> reports on several projects if that's what you mean.
> 
> >  Is the true evil the multiple chained getter calls?
> 
> Besides SEI CERT, there are many other reputable resources stating the same 
> problem with getenv.
> 
> https://www.ibm.com/support/knowledgecenter/SSLTBW_2.2.0/com.ibm.zos.v2r2.bpxbd00/getenv.htm
> https://www.keil.com/support/man/docs/armlib/armlib_chr1359122850667.htm
> https://pubs.opengroup.org/onlinepubs/009696799/functions/getenv.html
> https://pubs.opengroup.org/onlinepubs/7908799/xsh/getenv.html
> https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html
> https://man7.org/linux/man-pages/man3/getenv.3p.html
Actual problem is that getenv is returning a pointer to its internal static 
buffer instead of giving pointer to environ, that's why the string will change 
with a subsequent call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

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

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 335334.
zukatsinadze edited the summary of this revision.
zukatsinadze added a comment.

Gentle ping

- Diff with context
- Added some more tests
- Updated documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
  clang/test/Analysis/cert/env31-c.c
  clang/test/Analysis/cert/env34-c-cert-examples.c
  clang/test/Analysis/cert/env34-c.c

Index: clang/test/Analysis/cert/env34-c.c
===
--- /dev/null
+++ clang/test/Analysis/cert/env34-c.c
@@ -0,0 +1,331 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr\
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+
+typedef struct {
+  char * field;
+} lconv;
+lconv *localeconv(void);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char*, const char*);
+extern void foo(char *e);
+extern char* bar();
+
+
+void getenv_test1() {
+  char *p;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  *p; // no-warning, getenv result was assigned to the same pointer
+}
+
+void getenv_test2() {
+  char *p, *p2;
+
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test3() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  p3 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test4() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+  p3 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test5() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  p2 = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p3 = getenv("VAR3");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test6() {
+  char *p, *p2;
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR3");
+  // expected-note@-1{{previous function call was here}}
+  // expected-note@-2{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+
+  *p2; // no-warning
+
+  p = getenv("VAR4");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p; // no-warning
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test7() {
+  char *p, *p2;
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  foo(p);
+  // expected-warning@-1{{use of invalidated pointer 'p' in a function call}}
+  // expected-note@-2{{use of invalidated pointer 'p' in a function call}}
+}
+
+void getenv_test8() {
+  static const char *array[] = {
+ 0,
+ 0,
+ "/var/tmp",
+ "/usr/tmp",
+ "/tmp",
+ "."
+  };
+
+  if( !array[0] )
+  // expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{previous function call was here}}
+
+  if( !array[1] )
+  // expected-note@-1{{Taking true branch}}
+array[1] = getenv("TMPDIR");
+// expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *array

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze updated this revision to Diff 372237.
zukatsinadze added a comment.

Thanks for the review @martong

I've fixed all the suggestions.


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

https://reviews.llvm.org/D97699

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
  clang/test/Analysis/cert/env31-c.c
  clang/test/Analysis/cert/env34-c-cert-examples.c
  clang/test/Analysis/cert/env34-c.c

Index: clang/test/Analysis/cert/env34-c.c
===
--- /dev/null
+++ clang/test/Analysis/cert/env34-c.c
@@ -0,0 +1,331 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr\
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+
+#include "../Inputs/system-header-simulator.h"
+char *getenv(const char *name);
+char *setlocale(int category, const char *locale);
+char *strerror(int errnum);
+
+typedef struct {
+  char * field;
+} lconv;
+lconv *localeconv(void);
+
+typedef struct {
+} tm;
+char *asctime(const tm *timeptr);
+
+int strcmp(const char*, const char*);
+extern void foo(char *e);
+extern char* bar();
+
+
+void getenv_test1() {
+  char *p;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  *p; // no-warning, getenv result was assigned to the same pointer
+}
+
+void getenv_test2() {
+  char *p, *p2;
+
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test3() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  p3 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test4() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+  p3 = getenv("VAR3");
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test5() {
+  char *p, *p2, *p3;
+
+  p = getenv("VAR");
+  p2 = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  p3 = getenv("VAR3");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test6() {
+  char *p, *p2;
+  p = getenv("VAR");
+  *p; // no-warning
+
+  p = getenv("VAR2");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR3");
+  // expected-note@-1{{previous function call was here}}
+  // expected-note@-2{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+
+  *p2; // no-warning
+
+  p = getenv("VAR4");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *p; // no-warning
+  *p2;
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing an invalid pointer}}
+}
+
+void getenv_test7() {
+  char *p, *p2;
+  p = getenv("VAR");
+  // expected-note@-1{{previous function call was here}}
+  *p; // no-warning
+
+  p2 = getenv("VAR2");
+  // expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  foo(p);
+  // expected-warning@-1{{use of invalidated pointer 'p' in a function call}}
+  // expected-note@-2{{use of invalidated pointer 'p' in a function call}}
+}
+
+void getenv_test8() {
+  static const char *array[] = {
+ 0,
+ 0,
+ "/var/tmp",
+ "/usr/tmp",
+ "/tmp",
+ "."
+  };
+
+  if( !array[0] )
+  // expected-note@-1{{Taking true branch}}
+array[0] = getenv("TEMPDIR");
+// expected-note@-1{{previous function call was here}}
+
+  if( !array[1] )
+  // expected-note@-1{{Taking true branch}}
+array[1] = getenv("TMPDIR");
+// expected-note@-1{{'getenv' call may invalidate the the result of the previous 'getenv'}}
+
+  *array[0];
+  // expected-warning@-1{{dereferencing an invalid pointer}}
+  // expected-note@-2{{dereferencing a

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze marked 4 inline comments as done.
zukatsinadze added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:163
+// memory region returned by previous call of this function
+REGISTER_MAP_WITH_PROGRAMSTATE(PreviousCallResultMap, const char *,
+   const MemRegion *)

martong wrote:
> I think we could use the canonical `FunctionDecl*` as the key instead of 
> `const char *`. Then all the `eval` functions like `evalGetenv` and alike 
> could be substituted with one simple function.
Thanks! Those functions annoyed me a lot.


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

https://reviews.llvm.org/D97699

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