[clang][analyzer] Move checker 'cert.pos.34c' (in alpha.security) into 'PutenvStackArray' by balazske · Pull Request #92424 · llvm/llvm-project (original) (raw)

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

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.

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

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; + def PutenvWithAuto : Checker<"PutenvWithAuto">, + HelpText<"Finds calls to the 'putenv' function which pass a pointer to " + "an automatic variable as the argument.">, + Documentation; + } // 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;

} // 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");

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

+void free(void *); +void *malloc(size_t); +int putenv(char *); +int snprintf(char *, size_t, const char *, ...); + +int test_auto_var(const char *var) {

+} + +typedef struct {