[clang][analyzer] Move security.cert.env.InvalidPtr out of alpha

[gamesh411]

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. (Before the changes mentioned above, there were 27 reports, catching the getenv invalidates previous getenv results cases. That strict behaviour is disabled by default)

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Endre Fülöp (gamesh411)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/71912.diff

7 Files Affected:

• (modified) clang/docs/analyzer/checkers.rst (+69-69)
• (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+15-13)
• (modified) clang/test/Analysis/analyzer-config.c (+1-1)
• (modified) clang/test/Analysis/cert/env31-c.c (+5-5)
• (modified) clang/test/Analysis/cert/env34-c-cert-examples.c (+5-5)
• (modified) clang/test/Analysis/cert/env34-c.c (+2-2)
• (modified) clang/test/Analysis/invalid-ptr-checker.c (+4-4)

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"

[NagyDonat]

LGTM. The test results sound convincing and this is just a checker-rename commit that doesn't contain "actual" code changes that could've introduced bugs.

[whisperity]

What is moving out of alpha. here? This patch is adding some different checker to alpha. Where is the documentation for alpha.cert.pos.34c?

[whisperity]

There are no tests that exercise this checker, either.

[gamesh411]

The POS package was by default in the alpha hierarchy, and there was no POS package for non-alpha. For consistent naming, I have renamed the old one to POSAlpha and introduced a new one with the old name POS. This is why the diff is confusing.

[whisperity]

Why is the clang:dataflow label on this patch? I don't see where dataflow is used. Or is it used plain internally inside the otherwise unmodified checker?

[gamesh411]

cleaned up the committer email, as it was pointing to an old address