https://github.com/balazske created
https://github.com/llvm/llvm-project/pull/92424
The "cert" package looks not useful and the checker has not a meaningful name
with the old naming scheme.
Additionally tests and documentation is updated.
The checker looks good enough to be moved into non-alpha package.
From 769523d392204eac6c48cb80a2282212f3edbbe4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Fri, 10 May 2024 17:30:23 +0200
Subject: [PATCH] [clang][analyzer] Move checker alpha.security.cert.pos.34c
into security.PutenvWithAuto .
The "cert" package looks not useful and the checker has not a meaningful name
with the old naming scheme.
Additionally tests and documentation is updated.
The checker looks good enough to be moved into non-alpha package.
---
clang/docs/analyzer/checkers.rst | 97 +++++++++----------
.../clang/StaticAnalyzer/Checkers/Checkers.td | 10 +-
.../Analysis/cert/pos34-c-fp-suppression.cpp | 51 ----------
clang/test/Analysis/cert/pos34-c.cpp | 61 ------------
clang/test/Analysis/putenv-with-auto.c | 66 +++++++++++++
5 files changed, 119 insertions(+), 166 deletions(-)
delete mode 100644 clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
delete mode 100644 clang/test/Analysis/cert/pos34-c.cpp
create mode 100644 clang/test/Analysis/putenv-with-auto.c
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index eb8b58323da4d..6ea768d003378 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,54 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
}
+.. _security-putenv-with-auto:
+
+security.PutenvWithAuto
+"""""""""""""""""""""""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic
variable as the argument.
+Function ``putenv`` does not copy the passed string, only a pointer to the
data is stored.
+Content of an automatic variable is likely to be overwritten after returning
from the parent function.
+
+The problem can be solved by using a static variable or dynamically allocated
+memory. Even better is to avoid using ``putenv`` (it has other problems
+related to memory leaks) and use ``setenv`` instead.
+
+The check corresponds to CERT rule
+`POS34-C. Do not call putenv() with a pointer to an automatic variable as the
argument
+<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>`_.
+
+.. code-block:: c
+
+ int f() {
+ char[] env = "NAME=value";
+ return putenv(env); // putenv function should not be called with auto
variables
+ }
+
+Limitations:
+
+ - In specific cases one can pass automatic variables to ``putenv``,
+ but one needs to ensure that the given environment key stays
+ alive until it's removed or overwritten.
+ Since the analyzer cannot keep track if and when the string passed to
``putenv``
+ gets deallocated or overwritten, it needs to be slightly more aggressive
+ and warn for each case, leading in some cases to false-positive reports
like this:
+
+ .. code-block:: c
+
+ void baz() {
+ char env[] = "NAME=value";
+ putenv(env); // false-positive warning: putenv function should not
be called...
+ // More code...
+ // FIXME: It looks like that if one string was passed to putenv,
+ // it should not be deallocated at all, because when reading the
+ // environment variable a pointer into this string is returned.
+ // In this case, if another (or the same) thread reads variable
"NAME"
+ // at this point and does not copy the returned string, the data may
+ // become invalid.
+ putenv((char *)"NAME=anothervalue");
+ // This putenv call overwrites the previous entry, thus that can no
longer dangle.
+ } // 'env' array becomes dead only here.
+
.. _unix-checkers:
unix
@@ -2818,55 +2866,6 @@ alpha.security.cert
SEI CERT checkers which tries to find errors based on their `C coding rules
<https://wiki.sei.cmu.edu/confluence/display/c/2+Rules>`_.
-.. _alpha-security-cert-pos-checkers:
-
-alpha.security.cert.pos
-^^^^^^^^^^^^^^^^^^^^^^^
-
-SEI CERT checkers of `POSIX C coding rules
<https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152405>`_.
-
-.. _alpha-security-cert-pos-34c:
-
-alpha.security.cert.pos.34c
-"""""""""""""""""""""""""""
-Finds calls to the ``putenv`` function which pass a pointer to an automatic
variable as the argument.
-
-.. code-block:: c
-
- 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
- }
-
-Limitations:
-
- - Technically, one can pass automatic variables to ``putenv``,
- but one needs to ensure that the given environment key stays
- alive until it's removed or overwritten.
- Since the analyzer cannot keep track of which envvars get overwritten
- and when, it needs to be slightly more aggressive and warn for such
- cases too, leading in some cases to false-positive reports like this:
-
- .. code-block:: c
-
- void baz() {
- char env[] = "NAME=value";
- putenv(env); // false-positive warning: putenv function should not
be called...
- // More code...
- putenv((char *)"NAME=anothervalue");
- // This putenv call overwrites the previous entry, thus that can no
longer dangle.
- } // 'env' array becomes dead only here.
-
-alpha.security.cert.env
-^^^^^^^^^^^^^^^^^^^^^^^
-
-SEI CERT checkers of `Environment C coding rules
<https://wiki.sei.cmu.edu/confluence/x/JdcxBQ>`_.
-
alpha.security.taint
^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 64414e3d37f75..56aeeace66f9d 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
Dependencies<[SecuritySyntaxChecker]>,
Documentation<HasDocumentation>;
+ def PutenvWithAuto : Checker<"PutenvWithAuto">,
+ HelpText<"Finds calls to the 'putenv' function which pass a pointer to "
+ "an automatic variable as the argument.">,
+ Documentation<HasDocumentation>;
+
} // end "security"
let ParentPackage = ENV in {
@@ -1032,11 +1037,6 @@ let ParentPackage = ENV in {
let ParentPackage = POSAlpha in {
- def PutenvWithAuto : Checker<"34c">,
- HelpText<"Finds calls to the 'putenv' function which pass a pointer to "
- "an automatic variable as the argument.">,
- Documentation<HasDocumentation>;
-
} // end "alpha.cert.pos"
let ParentPackage = SecurityAlpha in {
diff --git a/clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
b/clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
deleted file mode 100644
index d982fcb8a1baf..0000000000000
--- a/clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
+++ /dev/null
@@ -1,51 +0,0 @@
-// 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
diff --git a/clang/test/Analysis/cert/pos34-c.cpp
b/clang/test/Analysis/cert/pos34-c.cpp
deleted file mode 100644
index f2bd7b393d889..0000000000000
--- a/clang/test/Analysis/cert/pos34-c.cpp
+++ /dev/null
@@ -1,61 +0,0 @@
-// 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
diff --git a/clang/test/Analysis/putenv-with-auto.c
b/clang/test/Analysis/putenv-with-auto.c
new file mode 100644
index 0000000000000..454185f6996a1
--- /dev/null
+++ b/clang/test/Analysis/putenv-with-auto.c
@@ -0,0 +1,66 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=security.PutenvWithAuto \
+// RUN: -verify %s
+
+#include "Inputs/system-header-simulator.h"
+void free(void *);
+void *malloc(size_t);
+int putenv(char *);
+int snprintf(char *, size_t, const char *, ...);
+
+int test_auto_var(const char *var) {
+ char env[1024];
+ (void)snprintf(env, sizeof(env), "TEST=%s", var);
+ return putenv(env); // expected-warning{{The 'putenv' function should not be
called with arguments that have automatic storage}}
+}
+
+int test_static_var(const char *var) {
+ static char env[1024];
+ (void)snprintf(env, sizeof(env), "TEST=%s", var);
+ return putenv(env);
+}
+
+void test_heap_memory(const char *var) {
+ const char *env_format = "TEST=%s";
+ const size_t len = strlen(var) + strlen(env_format);
+ char *env = (char *)malloc(len);
+ if (env == NULL)
+ return;
+ if (putenv(env) != 0) // no-warning: env was dynamically allocated.
+ free(env);
+}
+
+typedef struct {
+ int A;
+ char Env[1024];
+} Mem;
+
+int test_auto_var_struct() {
+ Mem mem;
+ return putenv(mem.Env); // expected-warning{{The 'putenv' function should
not be called with arguments that have automatic storage}}
+}
+
+int test_auto_var_subarray() {
+ char env[1024];
+ return putenv(env + 100); // expected-warning{{The 'putenv' function should
not be called with arguments that have automatic storage}}
+}
+
+int test_constant() {
+ char *env = "TEST";
+ return putenv(env);
+}
+
+extern char *ext_env;
+int test_extern() {
+ return putenv(ext_env); // no-warning: extern storage class.
+}
+
+void test_auto_var_reset() {
+ char env[] = "NAME=value";
+ // TODO: False Positive
+ putenv(env); // expected-warning{{The 'putenv' function should not be called
with arguments that have automatic storage}}
+ /*
+ ...
+ */
+ putenv((char *)"NAME=anothervalue");
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
