[clang][analyzer] Move checker 'cert.pos.34c' (in alpha.security) into 'PutenvStackArray' #92424
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ity.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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesThe "cert" package looks not useful and the checker has not a meaningful name with the old naming scheme. Full diff: https://github.com/llvm/llvm-project/pull/92424.diff 5 Files Affected:
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");
+}
|
steakhal
changed the title
Personally, I prefer reviewing content changes separately from deciding/moving the checker out of alpha. Here are the reasons for doing so:
I'd suggest to split this PR into two:
|
steakhal
reviewed
May 17, 2024
NagyDonat
reviewed
May 17, 2024
There was a problem hiding this comment.
Thanks for bringing this checker out of alpha! I like the new name and I agree that the old Limitations section was incorrect; and I have some minor suggestions in inline comments.
I'd also ask for running this checker on some open source projects; but as putenv is an outdated function, I'd guess that there would be no results.
| @@ -1032,11 +1037,6 @@ let ParentPackage = ENV in { | |||
|
|
|||
| let ParentPackage = POSAlpha in { | |||
There was a problem hiding this comment.
Please delete the packages that will no longer contain any checkers after this change. (As it's a bad naming scheme, they shouldn't be repopulated later.)
clang/docs/analyzer/checkers.rst
Outdated
Comment on lines
1220
to
1225
| // 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. |
There was a problem hiding this comment.
I agree with this remark.
I think you should just remove the whole Limitations: section and move this remark to the test file.
|
|
||
| void test_auto_var_reset() { | ||
| char env[] = "NAME=value"; | ||
| // TODO: False Positive |
There was a problem hiding this comment.
Suggested change
| // TODO: False Positive | |
| // If a 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. |
As you explained in the FIXME added in the documentation, this is not a false positive. It's a good idea to keep this testcase, but let's replace the old TODO comment with your reasoning for reporting this.
clang/docs/analyzer/checkers.rst
Outdated
|
|
||
| security.PutenvWithAuto | ||
| """"""""""""""""""""""" | ||
| Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument. |
There was a problem hiding this comment.
Suggested change
| Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument. | |
| Finds calls to the function ``putenv`` which pass a pointer to an automatic variable as the argument. |
|
I moved the checker to |
steakhal
approved these changes
May 18, 2024
|
Make sure you adjust/sync the commit title, content and the PR title before merging. |
balazske
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.