Skip to content

Commit b98a594

Browse files
gamesh411gamesh411
authored
[clang][analyzer] Move security.cert.env.InvalidPtr out of alpha (#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. (Before the changes mentioned above, there were 27 reports, catching the `getenv` invalidates previous `getenv` results cases. That strict behaviour is disabled by default)
1 parent dd0b3c2 commit b98a594

File tree

7 files changed

+107
-105
lines changed

7 files changed

+107
-105
lines changed

‎clang/docs/analyzer/checkers.rst

Lines changed: 69 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -755,6 +755,75 @@ security
755755
756756
Security related checkers.
757757
758+
.. _security-cert-env-InvalidPtr:
759+
760+
security.cert.env.InvalidPtr
761+
""""""""""""""""""""""""""""""""""
762+
763+
Corresponds to SEI CERT Rules `ENV31-C <https://wiki.sei.cmu.edu/confluence/display/c/ENV31-C.+Do+not+rely+on+an+environment+pointer+following+an+operation+that+may+invalidate+it>`_ and `ENV34-C <https://wiki.sei.cmu.edu/confluence/display/c/ENV34-C.+Do+not+store+pointers+returned+by+certain+functions>`_.
764+
765+
* **ENV31-C**:
766+
Rule is about the possible problem with ``main`` function's third argument, environment pointer,
767+
"envp". When environment array is modified using some modification function
768+
such as ``putenv``, ``setenv`` or others, It may happen that memory is reallocated,
769+
however "envp" is not updated to reflect the changes and points to old memory
770+
region.
771+
772+
* **ENV34-C**:
773+
Some functions return a pointer to a statically allocated buffer.
774+
Consequently, subsequent call of these functions will invalidate previous
775+
pointer. These functions include: ``getenv``, ``localeconv``, ``asctime``, ``setlocale``, ``strerror``
776+
777+
.. code-block:: c
778+
779+
int main(int argc, const char *argv[], const char *envp[]) {
780+
if (setenv("MY_NEW_VAR", "new_value", 1) != 0) {
781+
// setenv call may invalidate 'envp'
782+
/* Handle error */
783+
}
784+
if (envp != NULL) {
785+
for (size_t i = 0; envp[i] != NULL; ++i) {
786+
puts(envp[i]);
787+
// envp may no longer point to the current environment
788+
// this program has unanticipated behavior, since envp
789+
// does not reflect changes made by setenv function.
790+
}
791+
}
792+
return 0;
793+
}
794+
795+
void previous_call_invalidation() {
796+
char *p, *pp;
797+
798+
p = getenv("VAR");
799+
setenv("SOMEVAR", "VALUE", /*overwrite = */1);
800+
// call to 'setenv' may invalidate p
801+
802+
*p;
803+
// dereferencing invalid pointer
804+
}
805+
806+
807+
The ``InvalidatingGetEnv`` option is available for treating ``getenv`` calls as
808+
invalidating. When enabled, the checker issues a warning if ``getenv`` is called
809+
multiple times and their results are used without first creating a copy.
810+
This level of strictness might be considered overly pedantic for the commonly
811+
used ``getenv`` implementations.
812+
813+
To enable this option, use:
814+
``-analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true``.
815+
816+
By default, this option is set to *false*.
817+
818+
When this option is enabled, warnings will be generated for scenarios like the
819+
following:
820+
821+
.. code-block:: c
822+
823+
char* p = getenv("VAR");
824+
char* pp = getenv("VAR2"); // assumes this call can invalidate `env`
825+
strlen(p); // warns about accessing invalid ptr
826+
758827
.. _security-FloatLoopCounter:
759828
760829
security.FloatLoopCounter (C)
@@ -2549,75 +2618,6 @@ alpha.security.cert.env
25492618
25502619
SEI CERT checkers of `Environment C coding rules <https://wiki.sei.cmu.edu/confluence/x/JdcxBQ>`_.
25512620
2552-
.. _alpha-security-cert-env-InvalidPtr:
2553-
2554-
alpha.security.cert.env.InvalidPtr
2555-
""""""""""""""""""""""""""""""""""
2556-
2557-
Corresponds to SEI CERT Rules ENV31-C and ENV34-C.
2558-
2559-
ENV31-C:
2560-
Rule is about the possible problem with `main` function's third argument, environment pointer,
2561-
"envp". When environment array is modified using some modification function
2562-
such as putenv, setenv or others, It may happen that memory is reallocated,
2563-
however "envp" is not updated to reflect the changes and points to old memory
2564-
region.
2565-
2566-
ENV34-C:
2567-
Some functions return a pointer to a statically allocated buffer.
2568-
Consequently, subsequent call of these functions will invalidate previous
2569-
pointer. These functions include: getenv, localeconv, asctime, setlocale, strerror
2570-
2571-
.. code-block:: c
2572-
2573-
int main(int argc, const char *argv[], const char *envp[]) {
2574-
if (setenv("MY_NEW_VAR", "new_value", 1) != 0) {
2575-
// setenv call may invalidate 'envp'
2576-
/* Handle error */
2577-
}
2578-
if (envp != NULL) {
2579-
for (size_t i = 0; envp[i] != NULL; ++i) {
2580-
puts(envp[i]);
2581-
// envp may no longer point to the current environment
2582-
// this program has unanticipated behavior, since envp
2583-
// does not reflect changes made by setenv function.
2584-
}
2585-
}
2586-
return 0;
2587-
}
2588-
2589-
void previous_call_invalidation() {
2590-
char *p, *pp;
2591-
2592-
p = getenv("VAR");
2593-
setenv("SOMEVAR", "VALUE", /*overwrite = */1);
2594-
// call to 'setenv' may invalidate p
2595-
2596-
*p;
2597-
// dereferencing invalid pointer
2598-
}
2599-
2600-
2601-
The ``InvalidatingGetEnv`` option is available for treating getenv calls as
2602-
invalidating. When enabled, the checker issues a warning if getenv is called
2603-
multiple times and their results are used without first creating a copy.
2604-
This level of strictness might be considered overly pedantic for the commonly
2605-
used getenv implementations.
2606-
2607-
To enable this option, use:
2608-
``-analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true``.
2609-
2610-
By default, this option is set to *false*.
2611-
2612-
When this option is enabled, warnings will be generated for scenarios like the
2613-
following:
2614-
2615-
.. code-block:: c
2616-
2617-
char* p = getenv("VAR");
2618-
char* pp = getenv("VAR2"); // assumes this call can invalidate `env`
2619-
strlen(p); // warns about accessing invalid ptr
2620-
26212621
alpha.security.taint
26222622
^^^^^^^^^^^^^^^^^^^^
26232623

‎clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,12 @@ def InsecureAPI : Package<"insecureAPI">, ParentPackage<Security>;
7171
def SecurityAlpha : Package<"security">, ParentPackage<Alpha>;
7272
def Taint : Package<"taint">, ParentPackage<SecurityAlpha>;
7373

74-
def CERT : Package<"cert">, ParentPackage<SecurityAlpha>;
75-
def POS : Package<"pos">, ParentPackage<CERT>;
74+
def CERT : Package<"cert">, ParentPackage<Security>;
7675
def ENV : Package<"env">, ParentPackage<CERT>;
7776

77+
def CERTAlpha : Package<"cert">, ParentPackage<SecurityAlpha>;
78+
def POSAlpha : Package<"pos">, ParentPackage<CERTAlpha>;
79+
7880
def Unix : Package<"unix">;
7981
def UnixAlpha : Package<"unix">, ParentPackage<Alpha>;
8082
def CString : Package<"cstring">, ParentPackage<Unix>;
@@ -993,15 +995,6 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
993995

994996
} // end "security"
995997

996-
let ParentPackage = POS in {
997-
998-
def PutenvWithAuto : Checker<"34c">,
999-
HelpText<"Finds calls to the 'putenv' function which pass a pointer to "
1000-
"an automatic variable as the argument.">,
1001-
Documentation<HasDocumentation>;
1002-
1003-
} // end "alpha.cert.pos"
1004-
1005998
let ParentPackage = ENV in {
1006999

10071000
def InvalidPtrChecker : Checker<"InvalidPtr">,
@@ -1013,11 +1006,20 @@ let ParentPackage = ENV in {
10131006
"standard), which can lead to false positives depending on "
10141007
"implementation.",
10151008
"false",
1016-
InAlpha>,
1009+
Released>,
10171010
]>,
10181011
Documentation<HasDocumentation>;
10191012

1020-
} // end "alpha.cert.env"
1013+
} // end "security.cert.env"
1014+
1015+
let ParentPackage = POSAlpha in {
1016+
1017+
def PutenvWithAuto : Checker<"34c">,
1018+
HelpText<"Finds calls to the 'putenv' function which pass a pointer to "
1019+
"an automatic variable as the argument.">,
1020+
Documentation<HasDocumentation>;
1021+
1022+
} // end "alpha.cert.pos"
10211023

10221024
let ParentPackage = SecurityAlpha in {
10231025

‎clang/test/Analysis/analyzer-config.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
// CHECK-NEXT: alpha.osx.cocoa.DirectIvarAssignment:AnnotatedFunctions = false
1212
// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
1313
// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
14-
// CHECK-NEXT: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv = false
1514
// CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = ""
1615
// CHECK-NEXT: apply-fixits = false
1716
// CHECK-NEXT: assume-controlled-environment = false
@@ -116,6 +115,7 @@
116115
// CHECK-NEXT: region-store-small-array-limit = 5
117116
// CHECK-NEXT: region-store-small-struct-limit = 2
118117
// CHECK-NEXT: report-in-main-source-file = false
118+
// CHECK-NEXT: security.cert.env.InvalidPtr:InvalidatingGetEnv = false
119119
// CHECK-NEXT: serialize-stats = false
120120
// CHECK-NEXT: silence-checkers = ""
121121
// CHECK-NEXT: stable-report-filename = false

‎clang/test/Analysis/cert/env31-c.c

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,25 @@
11
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
2-
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
2+
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
33
// RUN: -verify=putenv,common \
44
// RUN: -DENV_INVALIDATING_CALL="putenv(\"X=Y\")"
55
//
66
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
7-
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
7+
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
88
// RUN: -verify=putenvs,common \
99
// RUN: -DENV_INVALIDATING_CALL="_putenv_s(\"X\", \"Y\")"
1010
//
1111
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
12-
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
12+
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
1313
// RUN: -verify=wputenvs,common \
1414
// RUN: -DENV_INVALIDATING_CALL="_wputenv_s(\"X\", \"Y\")"
1515
//
1616
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
17-
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
17+
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
1818
// RUN: -verify=setenv,common \
1919
// RUN: -DENV_INVALIDATING_CALL="setenv(\"X\", \"Y\", 0)"
2020
//
2121
// RUN: %clang_analyze_cc1 -analyzer-output=text -Wno-unused %s \
22-
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
22+
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
2323
// RUN: -verify=unsetenv,common \
2424
// RUN: -DENV_INVALIDATING_CALL="unsetenv(\"X\")"
2525

‎clang/test/Analysis/cert/env34-c-cert-examples.c

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
// Default options.
2-
// RUN: %clang_analyze_cc1 \
3-
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
2+
// RUN: %clang_analyze_cc1 \
3+
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
44
// RUN: -verify -Wno-unused %s
55
//
66
// Test the laxer handling of getenv function (this is the default).
7-
// RUN: %clang_analyze_cc1 \
8-
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
9-
// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
7+
// RUN: %clang_analyze_cc1 \
8+
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
9+
// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
1010
// RUN: -verify -Wno-unused %s
1111
//
1212
// Test the stricter handling of getenv function.
13-
// RUN: %clang_analyze_cc1 \
14-
// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
15-
// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
13+
// RUN: %clang_analyze_cc1 \
14+
// RUN: -analyzer-checker=core,security.cert.env.InvalidPtr \
15+
// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
1616
// RUN: -verify=expected,pedantic -Wno-unused %s
1717

1818
#include "../Inputs/system-header-simulator.h"

‎clang/test/Analysis/cert/env34-c.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// RUN: %clang_analyze_cc1 \
2-
// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr\
3-
// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
2+
// RUN: -analyzer-checker=security.cert.env.InvalidPtr\
3+
// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
44
// RUN: -analyzer-output=text -verify -Wno-unused %s
55

66
#include "../Inputs/system-header-simulator.h"

‎clang/test/Analysis/invalid-ptr-checker.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
// RUN: %clang_analyze_cc1 \
2-
// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr \
3-
// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
1+
// RUN: %clang_analyze_cc1 \
2+
// RUN: -analyzer-checker=security.cert.env.InvalidPtr \
3+
// RUN: -analyzer-config security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
44
// RUN: -analyzer-output=text -verify -Wno-unused %s
55
//
6-
// RUN: %clang_analyze_cc1 \
7-
// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr \
8-
// RUN: -analyzer-config \
9-
// RUN: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
6+
// RUN: %clang_analyze_cc1 \
7+
// RUN: -analyzer-checker=security.cert.env.InvalidPtr \
8+
// RUN: -analyzer-config \
9+
// RUN: security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
1010
// RUN: -analyzer-output=text -verify=expected,pedantic -Wno-unused %s
1111

1212
#include "Inputs/system-header-simulator.h"

0 commit comments

Comments
 (0)