https://github.com/gamesh411 created https://github.com/llvm/llvm-project/pull/71912
Thanks to recent improvements in #67663, InvalidPtr checker does not emit any false positives on the following OS projects: memcached, tmux, curl, twin, vim, openssl, sqlite, ffmpeg, postgres, tinyxml2, libwebm, xerces, bitcoin, protobuf, qtbase, contour, acid, openrct2. From 2d94271affd27c5ebf1073a9effbe6c7815f5c01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fu...@sigmatechnology.com> Date: Fri, 10 Nov 2023 10:08:58 +0100 Subject: [PATCH] [analyzer] Move security.cert.env.InvalidPtr out of alpha Thanks to recent improvements in #67663, InvalidPtr checker does not emit any false positives on the following OS projects: memcached, tmux, curl, twin, vim, openssl, sqlite, ffmpeg, postgres, tinyxml2, libwebm, xerces, bitcoin, protobuf, qtbase, contour, acid, openrct2 --- clang/docs/analyzer/checkers.rst | 138 +++++++++--------- .../clang/StaticAnalyzer/Checkers/Checkers.td | 28 ++-- clang/test/Analysis/analyzer-config.c | 2 +- clang/test/Analysis/cert/env31-c.c | 10 +- .../Analysis/cert/env34-c-cert-examples.c | 10 +- clang/test/Analysis/cert/env34-c.c | 4 +- clang/test/Analysis/invalid-ptr-checker.c | 8 +- 7 files changed, 101 insertions(+), 99 deletions(-) diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 43137f4b020f9f7..ff4559aa89d96a0 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -755,6 +755,75 @@ security Security related checkers. +.. _security-cert-env-InvalidPtr: + +security.cert.env.InvalidPtr +"""""""""""""""""""""""""""""""""" + +Corresponds to SEI CERT Rules ENV31-C and ENV34-C. + +ENV31-C: +Rule is about the possible problem with `main` function's third argument, environment pointer, +"envp". When environment array is modified using some modification function +such as putenv, setenv or others, It may happen that memory is reallocated, +however "envp" is not updated to reflect the changes and points to old memory +region. + +ENV34-C: +Some functions return a pointer to a statically allocated buffer. +Consequently, subsequent call of these functions will invalidate previous +pointer. These functions include: getenv, localeconv, asctime, setlocale, strerror + +.. code-block:: c + + int main(int argc, const char *argv[], const char *envp[]) { + if (setenv("MY_NEW_VAR", "new_value", 1) != 0) { + // setenv call may invalidate 'envp' + /* Handle error */ + } + if (envp != NULL) { + for (size_t i = 0; envp[i] != NULL; ++i) { + puts(envp[i]); + // envp may no longer point to the current environment + // this program has unanticipated behavior, since envp + // does not reflect changes made by setenv function. + } + } + return 0; + } + + void previous_call_invalidation() { + char *p, *pp; + + p = getenv("VAR"); + setenv("SOMEVAR", "VALUE", /*overwrite = */1); + // call to 'setenv' may invalidate p + + *p; + // dereferencing invalid pointer + } + + +The ``InvalidatingGetEnv`` option is available for treating getenv calls as +invalidating. When enabled, the checker issues a warning if getenv is called +multiple times and their results are used without first creating a copy. +This level of strictness might be considered overly pedantic for the commonly +used getenv implementations. + +To enable this option, use: +``-analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true``. + +By default, this option is set to *false*. + +When this option is enabled, warnings will be generated for scenarios like the +following: + +.. code-block:: c + + char* p = getenv("VAR"); + char* pp = getenv("VAR2"); // assumes this call can invalidate `env` + strlen(p); // warns about accessing invalid ptr + .. _security-FloatLoopCounter: security.FloatLoopCounter (C) @@ -2479,75 +2548,6 @@ alpha.security.cert.env SEI CERT checkers of `Environment C coding rules <https://wiki.sei.cmu.edu/confluence/x/JdcxBQ>`_. -.. _alpha-security-cert-env-InvalidPtr: - -alpha.security.cert.env.InvalidPtr -"""""""""""""""""""""""""""""""""" - -Corresponds to SEI CERT Rules ENV31-C and ENV34-C. - -ENV31-C: -Rule is about the possible problem with `main` function's third argument, environment pointer, -"envp". When environment array is modified using some modification function -such as putenv, setenv or others, It may happen that memory is reallocated, -however "envp" is not updated to reflect the changes and points to old memory -region. - -ENV34-C: -Some functions return a pointer to a statically allocated buffer. -Consequently, subsequent call of these functions will invalidate previous -pointer. These functions include: getenv, localeconv, asctime, setlocale, strerror - -.. code-block:: c - - int main(int argc, const char *argv[], const char *envp[]) { - if (setenv("MY_NEW_VAR", "new_value", 1) != 0) { - // setenv call may invalidate 'envp' - /* Handle error */ - } - if (envp != NULL) { - for (size_t i = 0; envp[i] != NULL; ++i) { - puts(envp[i]); - // envp may no longer point to the current environment - // this program has unanticipated behavior, since envp - // does not reflect changes made by setenv function. - } - } - return 0; - } - - void previous_call_invalidation() { - char *p, *pp; - - p = getenv("VAR"); - setenv("SOMEVAR", "VALUE", /*overwrite = */1); - // call to 'setenv' may invalidate p - - *p; - // dereferencing invalid pointer - } - - -The ``InvalidatingGetEnv`` option is available for treating getenv calls as -invalidating. When enabled, the checker issues a warning if getenv is called -multiple times and their results are used without first creating a copy. -This level of strictness might be considered overly pedantic for the commonly -used getenv implementations. - -To enable this option, use: -``-analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true``. - -By default, this option is set to *false*. - -When this option is enabled, warnings will be generated for scenarios like the -following: - -.. code-block:: c - - char* p = getenv("VAR"); - char* pp = getenv("VAR2"); // assumes this call can invalidate `env` - strlen(p); // warns about accessing invalid ptr - alpha.security.taint ^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index b6e9f0fae1c7f48..34edda022375cab 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -71,10 +71,12 @@ def InsecureAPI : Package<"insecureAPI">, ParentPackage<Security>; def SecurityAlpha : Package<"security">, ParentPackage<Alpha>; def Taint : Package<"taint">, ParentPackage<SecurityAlpha>; -def CERT : Package<"cert">, ParentPackage<SecurityAlpha>; -def POS : Package<"pos">, ParentPackage<CERT>; +def CERT : Package<"cert">, ParentPackage<Security>; def ENV : Package<"env">, ParentPackage<CERT>; +def CERTAlpha : Package<"cert">, ParentPackage<SecurityAlpha>; +def POSAlpha : Package<"pos">, ParentPackage<CERTAlpha>; + def Unix : Package<"unix">; def UnixAlpha : Package<"unix">, ParentPackage<Alpha>; def CString : Package<"cstring">, ParentPackage<Unix>; @@ -989,15 +991,6 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">, } // end "security" -let ParentPackage = POS 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 = ENV in { def InvalidPtrChecker : Checker<"InvalidPtr">, @@ -1009,11 +1002,20 @@ let ParentPackage = ENV in { "standard), which can lead to false positives depending on " "implementation.", "false", - InAlpha>, + Released>, ]>, Documentation<HasDocumentation>; -} // end "alpha.cert.env" +} // 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 { diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c index 8abfbf84983d811..45adbb0348d1864 100644 --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -11,7 +11,6 @@ // CHECK-NEXT: alpha.osx.cocoa.DirectIvarAssignment:AnnotatedFunctions = false // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01 -// CHECK-NEXT: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv = false // CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = "" // CHECK-NEXT: alpha.unix.Errno:AllowErrnoReadOutsideConditionExpressions = true // CHECK-NEXT: apply-fixits = false @@ -117,6 +116,7 @@ // CHECK-NEXT: region-store-small-array-limit = 5 // CHECK-NEXT: region-store-small-struct-limit = 2 // CHECK-NEXT: report-in-main-source-file = false +// CHECK-NEXT: security.cert.env.InvalidPtr:InvalidatingGetEnv = false // CHECK-NEXT: serialize-stats = false // CHECK-NEXT: silence-checkers = "" // CHECK-NEXT: stable-report-filename = false diff --git a/clang/test/Analysis/cert/env31-c.c b/clang/test/Analysis/cert/env31-c.c index feaa2baefea7c72..32908c5576cb854 100644 --- a/clang/test/Analysis/cert/env31-c.c +++ b/clang/test/Analysis/cert/env31-c.c @@ -1,25 +1,25 @@ // RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \ -// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \ // RUN: -verify=putenv,common \ // RUN: -DENV_INVALIDATING_CALL="putenv(\"X=Y\")" // // RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \ -// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \ // RUN: -verify=putenvs,common \ // RUN: -DENV_INVALIDATING_CALL="_putenv_s(\"X\", \"Y\")" // // RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \ -// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \ // RUN: -verify=wputenvs,common \ // RUN: -DENV_INVALIDATING_CALL="_wputenv_s(\"X\", \"Y\")" // // RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \ -// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \ // RUN: -verify=setenv,common \ // RUN: -DENV_INVALIDATING_CALL="setenv(\"X\", \"Y\", 0)" // // RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \ -// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \ // RUN: -verify=unsetenv,common \ // RUN: -DENV_INVALIDATING_CALL="unsetenv(\"X\")" diff --git a/clang/test/Analysis/cert/env34-c-cert-examples.c b/clang/test/Analysis/cert/env34-c-cert-examples.c index 6d6659e55d86b93..54d9a3a62c89d6a 100644 --- a/clang/test/Analysis/cert/env34-c-cert-examples.c +++ b/clang/test/Analysis/cert/env34-c-cert-examples.c @@ -1,18 +1,18 @@ // Default options. // RUN: %clang_analyze_cc1 \ -// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \ // RUN: -verify -Wno-unused %s // // Test the laxer handling of getenv function (this is the default). // RUN: %clang_analyze_cc1 \ -// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ -// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \ +// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \ +// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=false \ // RUN: -verify -Wno-unused %s // // Test the stricter handling of getenv function. // RUN: %clang_analyze_cc1 \ -// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ -// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ +// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \ +// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ // RUN: -verify=expected,pedantic -Wno-unused %s #include "../Inputs/system-header-simulator.h" diff --git a/clang/test/Analysis/cert/env34-c.c b/clang/test/Analysis/cert/env34-c.c index 94bc2cf95ccc9b0..d307f0d8f4bb01f 100644 --- a/clang/test/Analysis/cert/env34-c.c +++ b/clang/test/Analysis/cert/env34-c.c @@ -1,6 +1,6 @@ // RUN: %clang_analyze_cc1 \ -// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr\ -// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ +// RUN: -analyzer-checker=security.cert.env.InvalidPtr\ +// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ // RUN: -analyzer-output=text -verify -Wno-unused %s #include "../Inputs/system-header-simulator.h" diff --git a/clang/test/Analysis/invalid-ptr-checker.c b/clang/test/Analysis/invalid-ptr-checker.c index e8ffee7fb479dc9..b12af9806e1daf5 100644 --- a/clang/test/Analysis/invalid-ptr-checker.c +++ b/clang/test/Analysis/invalid-ptr-checker.c @@ -1,12 +1,12 @@ // RUN: %clang_analyze_cc1 \ -// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr \ -// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \ +// RUN: -analyzer-checker=security.cert.env.InvalidPtr \ +// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=false \ // RUN: -analyzer-output=text -verify -Wno-unused %s // // RUN: %clang_analyze_cc1 \ -// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-checker=security.cert.env.InvalidPtr \ // RUN: -analyzer-config \ -// RUN: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ +// RUN: security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ // RUN: -analyzer-output=text -verify=expected,pedantic -Wno-unused %s #include "Inputs/system-header-simulator.h" _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits