[clang] 3c23047 - [clang][analyzer] Move checker 'cert.pos.34c' (in alpha.security) into 'PutenvStackArray' (#92424)
via cfe-commits
cfe-commits at lists.llvm.org
Thu May 23 03:56:20 PDT 2024
Author: Balázs Kéri
Date: 2024-05-23T12:56:16+02:00
New Revision: 3c23047413957024294872e38c27707c71d05805
URL: https://github.com/llvm/llvm-project/commit/3c23047413957024294872e38c27707c71d05805
DIFF: https://github.com/llvm/llvm-project/commit/3c23047413957024294872e38c27707c71d05805.diff
LOG: [clang][analyzer] Move checker 'cert.pos.34c' (in alpha.security) into 'PutenvStackArray' (#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.
Added:
clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp
clang/test/Analysis/putenv-stack-array.c
Modified:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
Removed:
clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
clang/test/Analysis/cert/pos34-c-fp-suppression.cpp
clang/test/Analysis/cert/pos34-c.cpp
################################################################################
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index b4bd9dac1cbc2..ac9f0b06f63ba 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2833,6 +2833,31 @@ Warn on mmap() calls that are both writable and executable.
// code
}
+.. _alpha-security-putenv-stack-array:
+
+alpha.security.PutenvStackArray (C)
+"""""""""""""""""""""""""""""""""""
+Finds calls to the ``putenv`` function which pass a pointer to a stack-allocated
+(automatic) array as the argument. Function ``putenv`` does not copy the passed
+string, only a pointer to the data is stored and this data can be read even by
+other threads. Content of a stack-allocated array is likely to be overwritten
+after returning from the parent function.
+
+The problem can be solved by using a static array 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 stack-allocated string
+ }
+
.. _alpha-security-ReturnPtrRange:
alpha.security.ReturnPtrRange (C)
@@ -2859,55 +2884,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 d0ba1ce548403..40f443047bd4b 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1035,15 +1035,6 @@ let ParentPackage = ENV in {
} // end "security.cert.env"
-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 {
def ArrayBoundChecker : Checker<"ArrayBound">,
@@ -1054,10 +1045,6 @@ def ArrayBoundCheckerV2 : Checker<"ArrayBoundV2">,
HelpText<"Warn about buffer overflows (newer checker)">,
Documentation<HasDocumentation>;
-def ReturnPointerRangeChecker : Checker<"ReturnPtrRange">,
- HelpText<"Check for an out-of-bound pointer being returned to callers">,
- Documentation<HasDocumentation>;
-
def MallocOverflowSecurityChecker : Checker<"MallocOverflow">,
HelpText<"Check for overflows in the arguments to malloc()">,
Documentation<HasDocumentation>;
@@ -1078,6 +1065,15 @@ def MmapWriteExecChecker : Checker<"MmapWriteExec">,
]>,
Documentation<HasDocumentation>;
+def PutenvStackArray : Checker<"PutenvStackArray">,
+ HelpText<"Finds calls to the function 'putenv' which pass a pointer to "
+ "an automatic (stack-allocated) array as the argument.">,
+ Documentation<HasDocumentation>;
+
+def ReturnPointerRangeChecker : Checker<"ReturnPtrRange">,
+ HelpText<"Check for an out-of-bound pointer being returned to callers">,
+ Documentation<HasDocumentation>;
+
} // end "alpha.security"
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 45d3788f105dc..cd5a3bdd02e4a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -96,7 +96,7 @@ add_clang_library(clangStaticAnalyzerCheckers
PointerSortingChecker.cpp
PointerSubChecker.cpp
PthreadLockChecker.cpp
- cert/PutenvWithAutoChecker.cpp
+ PutenvStackArrayChecker.cpp
RetainCountChecker/RetainCountChecker.cpp
RetainCountChecker/RetainCountDiagnostics.cpp
ReturnPointerRangeChecker.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp
similarity index 70%
rename from clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
rename to clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp
index a82f7caf16b29..d59cebf0aa5cb 100644
--- a/clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp
@@ -1,4 +1,4 @@
-//== PutenvWithAutoChecker.cpp --------------------------------- -*- C++ -*--=//
+//== PutenvStackArrayChecker.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.
@@ -6,13 +6,13 @@
//
//===----------------------------------------------------------------------===//
//
-// This file defines PutenvWithAutoChecker which finds calls of ``putenv``
-// function with automatic variable as the argument.
+// This file defines PutenvStackArrayChecker which finds calls of ``putenv``
+// function with automatic array variable as the argument.
// https://wiki.sei.cmu.edu/confluence/x/6NYxBQ
//
//===----------------------------------------------------------------------===//
-#include "../AllocationState.h"
+#include "AllocationState.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -26,9 +26,9 @@ using namespace clang;
using namespace ento;
namespace {
-class PutenvWithAutoChecker : public Checker<check::PostCall> {
+class PutenvStackArrayChecker : public Checker<check::PostCall> {
private:
- BugType BT{this, "'putenv' function should not be called with auto variables",
+ BugType BT{this, "'putenv' called with stack-allocated string",
categories::SecurityError};
const CallDescription Putenv{CDM::CLibrary, {"putenv"}, 1};
@@ -37,8 +37,8 @@ class PutenvWithAutoChecker : public Checker<check::PostCall> {
};
} // namespace
-void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call,
- CheckerContext &C) const {
+void PutenvStackArrayChecker::checkPostCall(const CallEvent &Call,
+ CheckerContext &C) const {
if (!Putenv.matches(Call))
return;
@@ -50,7 +50,7 @@ void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call,
return;
StringRef ErrorMsg = "The 'putenv' function should not be called with "
- "arguments that have automatic storage";
+ "arrays that have automatic storage";
ExplodedNode *N = C.generateErrorNode();
auto Report = std::make_unique<PathSensitiveBugReport>(BT, ErrorMsg, N);
@@ -60,8 +60,10 @@ void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call,
C.emitReport(std::move(Report));
}
-void ento::registerPutenvWithAuto(CheckerManager &Mgr) {
- Mgr.registerChecker<PutenvWithAutoChecker>();
+void ento::registerPutenvStackArray(CheckerManager &Mgr) {
+ Mgr.registerChecker<PutenvStackArrayChecker>();
}
-bool ento::shouldRegisterPutenvWithAuto(const CheckerManager &) { return true; }
+bool ento::shouldRegisterPutenvStackArray(const CheckerManager &) {
+ return true;
+}
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 at -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 at -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-stack-array.c b/clang/test/Analysis/putenv-stack-array.c
new file mode 100644
index 0000000000000..fbbf93259ab85
--- /dev/null
+++ b/clang/test/Analysis/putenv-stack-array.c
@@ -0,0 +1,70 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=alpha.security.PutenvStackArray \
+// 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 arrays 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); // no-warning: static array is used
+}
+
+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}}
+}
+
+int test_auto_var_subarray() {
+ char env[1024];
+ return putenv(env + 100); // expected-warning{{The 'putenv' function should not be called with}}
+}
+
+int test_constant() {
+ char *env = "TEST";
+ return putenv(env); // no-warning: data is not on the stack
+}
+
+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";
+ putenv(env); // expected-warning{{The 'putenv' function should not be called with}}
+ // ... (do something)
+ // Even cases like this are likely a bug:
+ // 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");
+}
More information about the cfe-commits
mailing list