[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";

[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] D71433: [analyzer] CERT: POS34-C

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

Szelethus added a comment.
Herald added a subscriber: martong.

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


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 = 

[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

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

Szelethus accepted this revision.
Szelethus added a comment.

This is a very neat checker, the source code reads so easily, we might as well 
put it as the official CERT rule description.

I think adding the non-compliant and compliant code examples would be nice. I 
also wrote some inline comments, but I'm fine with leaving them for a later 
patch. LGTM, granted that @NoQ and @Charusso are happy.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h:16-22
+extern const char *const CoreFoundationObjectiveC;
+extern const char *const LogicError;
+extern const char *const MemoryRefCount;
+extern const char *const MemoryError;
+extern const char *const UnixAPI;
+extern const char *const CXXObjectLifecycle;
+extern const char *const SecurityError;

We could turns these into `llvm::StringLiteral`s one day.



Comment at: clang/lib/StaticAnalyzer/Checkers/AllocationState.h:28-30
+/// \returns The MallocBugVisitor.
+std::unique_ptr getMallocBRVisitor(SymbolRef Sym);
+

Is this deadcode now?



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

And this?



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:50
+  StringRef ErrorMsg =
+  "'putenv' function should not be called with auto variables";
+  ExplodedNode *N = C.generateErrorNode();

How about `The 'putenv' function should not be called with arguments that have 
automatic storage`. But I guess we could leave wordsmithing for later.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:55
+  // Track the argument.
+  if (isa(MSR)) {
+bugreporter::trackExpressionValue(Report->getErrorNode(), ArgExpr, 
*Report);

This should always be true, since we bail out a couple lines up if it isn't, 
right?


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#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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

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?


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 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 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:57-58
+bugreporter::trackExpressionValue(Report->getErrorNode(), ArgExpr, 
*Report);
+  } else if (const SymbolRef Sym =
+ ArgV.getAsSymbol()) { // It is a `HeapSpaceRegion`
+Report->addVisitor(allocation_state::getMallocBRVisitor(Sym));

This is impossible because `StackSpaceRegion` and `HeapSpaceRegion` do not 
overlap and above you checked that it's the former.



Comment at: clang/test/Analysis/cert/pos34-c.cpp:6
+// 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
+

Btw - CERT has minified links!

{F11286962}

{F11286963}


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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

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 after the 10.0.0 release-tag has been set, so both of the patches 
could land in LLVM 11. We have not seen any `putenv()` in the wild yet.


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

[Csaba Dabis via Phabricator via cfe-commits]

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

I have created a `notes.cpp` test-file to test the notes in my checkers, but I 
think this checker is fine without that test file. @NoQ, what do you think?


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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D71433#1784920 , @zukatsinadze 
wrote:

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


Let's get the initial attempt right first, and delay this for the next patch. 
You could accomplish this by keeping track of the last `putenv()` in a program 
state trait and moving the warning in `checkEndFunction()`.




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}}

zukatsinadze wrote:
> I need `isPossiblyAutoVar` for this type. 
This test is pretty questionable. There is no indication in the code that `a` 
points to an automatic variable.



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

zukatsinadze wrote:
> And `GlobalInternalSpaceRegion` for this.
This test is wrong. `"hello"` is not an automatic variable.


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 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 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 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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.




Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:27
+class PutenvWithAutoChecker : public Checker {
+  mutable std::unique_ptr BT;
+

The modern idiom is `BugType BT{this, "Bug kind", "Bug category"};` - and drop 
the lazy initialization as well.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:35-37
+  if (const IdentifierInfo *II = Call.getCalleeIdentifier())
+if (!II->isStr("putenv"))
+  return;

The modern idiom here is `CallDescription`.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:42-43
+  const Expr *ArgExpr = Call.getArgExpr(0);
+  const MemSpaceRegion *MSR =
+  ArgV.getAsRegion()->getBaseRegion()->getMemorySpace();
+

You can call `getMemorySpace()` directly on any region.



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) ||

Simply check whether the memory space is a stack memory space. This should be a 
one-liner.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:58
+this, "'putenv' function should not be called with auto variables",
+categories::SecurityError));
+  ExplodedNode *N = C.generateErrorNode();

Charusso wrote:
> @NoQ, it is from your documentation. Would we prefer that or the 
> `registerChecker(CheckerManager &)` is the new way to construct the 
> `BugType`? I believe the latter is more appropriate.
Replied above^^



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:60
+  ExplodedNode *N = C.generateErrorNode();
+  auto Report = llvm::make_unique(*BT, BT->getName(), N);
+  C.emitReport(std::move(Report));

Charusso wrote:
> We would like to point out the allocation's site, where it comes from.
> We have two facilities to do so: `MallocBugVisitor` to track dynamic 
> allocation and `trackExpressionValue()` to track any kind of an expression.
> 
> You could find examples from my CERT-checker: D70411. The rough idea is that:
> ```lang=c
> // Track the argument.
> if (isa(MSR)) {
>   bugreporter::trackExpressionValue(C.getPredecessor(), ArgExpr, *Report);
> } else if (const SymbolRef Sym = ArgV.getAsSymbol()) { // It is a 
> `HeapSpaceRegion`
>   Report->addVisitor(allocation_state::getMallocBRVisitor(Sym));
> }
> ```
> ~ here you need to copy-paste the `getMallocBRVisitor()` from my review. 
> Sorry!
I'm not sure it's valid to use `C.getPredecessor()` for tracking; it might get 
you into trouble for the same reason that using `C.getPredecessor()` as error 
node will get you into trouble. Please use the error node itself instead.


Repository:
  rC Clang

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a reviewer: aaron.ballman.
Charusso added a subscriber: aaron.ballman.
Charusso added inline comments.



Comment at: clang/docs/analyzer/checkers.rst:1881
+
+  #include 
+

I would remove that line.



Comment at: clang/docs/analyzer/checkers.rst:1893
+
+This check corresponds to the CERT C Coding Standard rule.
+

I think it is clear it is a CERT-checker.



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;

I would write ##`putenv`## -> `'putenv'` and the CERT rule-number should be 
clear from that point so you could emit it.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:52
+   isa(MSR) ||
+   isa(MSR) || isa(MSR)))
+return;

I think you could shrink that into:
```lang=c
!IsPossiblyAutoVar && !isa(MSR)
```

What you have specified is a negation of 
```lang=c
!isa(MSR) && !isa(MSR) && 
!isa(MSR)
```
~ according to: 
https://clang.llvm.org/doxygen/classclang_1_1ento_1_1MemSpaceRegion.html

where the `CodeSpaceRegion` and `GlobalInternalSpaceRegion` is not that 
interesting for the checker.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:58
+this, "'putenv' function should not be called with auto variables",
+categories::SecurityError));
+  ExplodedNode *N = C.generateErrorNode();

@NoQ, it is from your documentation. Would we prefer that or the 
`registerChecker(CheckerManager &)` is the new way to construct the `BugType`? 
I believe the latter is more appropriate.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:60
+  ExplodedNode *N = C.generateErrorNode();
+  auto Report = llvm::make_unique(*BT, BT->getName(), N);
+  C.emitReport(std::move(Report));

We would like to point out the allocation's site, where it comes from.
We have two facilities to do so: `MallocBugVisitor` to track dynamic allocation 
and `trackExpressionValue()` to track any kind of an expression.

You could find examples from my CERT-checker: D70411. The rough idea is that:
```lang=c
// Track the argument.
if (isa(MSR)) {
  bugreporter::trackExpressionValue(C.getPredecessor(), ArgExpr, *Report);
} else if (const SymbolRef Sym = ArgV.getAsSymbol()) { // It is a 
`HeapSpaceRegion`
  Report->addVisitor(allocation_state::getMallocBRVisitor(Sym));
}
```
~ here you need to copy-paste the `getMallocBRVisitor()` from my review. Sorry!



Comment at: clang/test/Analysis/cert/pos34-c.cpp:4
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"

Could you inject the rule's page please?



Comment at: clang/test/Analysis/cert/pos34-c.cpp:13
+
+// example from cert
+int volatile_memory1(const char *var) {

I would name the rule properly, as a sentence like: `Example from the CERT 
rule's page.`

My idea was that to have a `/cert/pos34-c.cpp` which only consists of the 
rule's page stuff mentioning the rule's page on the top. And having a separate 
file which only consists of arbitrary tests called like 
`/cert/pos34-c-fp-suppression.cpp` as it shows some false-positive suppression 
what you have found on tests or something. Like @aaron.ballman wrote two tests 
in D70823 and I would copy-paste them from his comment.


Repository:
  rC Clang

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 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: