steakhal created this revision.
steakhal added reviewers: NoQ, martong, ASDenysPetrov, balazske, Szelethus,
gamesh411.
Herald added subscribers: manas, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
steakhal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Previously, system globals were treated as immutable regions, unless it
was the `errno` which is known to be frequently modified.
D124244 <https://reviews.llvm.org/D124244> wants to add a check for stores to
immutable regions.
It would basically turn all stores to system globals into an error even
though we have no reason to believe that those mutable sys globals
should be treated as if they were immutable. And this leads to
false-positives if we apply D124244 <https://reviews.llvm.org/D124244>.
In this patch, I'm proposing to treat mutable sys globals actually
mutable, hence allocate them into the `GlobalSystemSpaceRegion`, UNLESS
they were declared as `const` (and a primitive arithmetic type), in
which case, we should use `GlobalImmutableSpaceRegion`.
In any other cases, I'm using the `GlobalInternalSpaceRegion`, which is
no different than the previous behavior.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D127306
Files:
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
clang/test/Analysis/Inputs/some_system_globals.h
clang/test/Analysis/Inputs/some_user_globals.h
clang/test/Analysis/global-region-invalidation.c
clang/test/Analysis/globals-are-not-always-immutable.c
Index: clang/test/Analysis/globals-are-not-always-immutable.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/globals-are-not-always-immutable.c
@@ -0,0 +1,56 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -analyzer-checker=core,debug.ExprInspection
+
+#include "Inputs/errno_var.h"
+#include "Inputs/some_system_globals.h"
+#include "Inputs/some_user_globals.h"
+
+void invalidate_globals(void);
+void clang_analyzer_eval(int x);
+
+/// Test the special system 'errno'
+void test_errno() {
+ errno = 2;
+ clang_analyzer_eval(errno == 2); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(errno == 2); // expected-warning {{UNKNOWN}}
+}
+
+/// Test user global variables
+void test_my_const_user_global() {
+ if (my_const_user_global != 2)
+ return;
+
+ clang_analyzer_eval(my_const_user_global == 2); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(my_const_user_global == 2); // expected-warning {{TRUE}}
+}
+
+void test_my_mutable_user_global() {
+ if (my_mutable_user_global != 2)
+ return;
+
+ clang_analyzer_eval(my_mutable_user_global == 2); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(my_mutable_user_global == 2); // expected-warning {{UNKNOWN}}
+}
+
+/// Test system global variables
+void test_my_const_system_global() {
+ if (my_const_system_global != 2)
+ return;
+
+ clang_analyzer_eval(my_const_system_global == 2); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(my_const_system_global == 2); // expected-warning {{TRUE}}
+}
+
+void test_my_mutable_system_global() {
+ if (my_mutable_system_global != 2)
+ return;
+
+ clang_analyzer_eval(my_mutable_system_global == 2); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(my_mutable_system_global == 2); // expected-warning {{UNKNOWN}} It was previously TRUE.
+}
Index: clang/test/Analysis/global-region-invalidation.c
===================================================================
--- clang/test/Analysis/global-region-invalidation.c
+++ clang/test/Analysis/global-region-invalidation.c
@@ -28,11 +28,17 @@
void foo(void);
int stdinTest(void) {
int i = 0;
- fscanf(stdin, "%d", &i);
+ fscanf(stdin, "%d", &i); // (1)
foo();
int m = i; // expected-warning + {{tainted}}
- fscanf(stdin, "%d", &i);
- int j = i; // expected-warning + {{tainted}}
+ fscanf(stdin, "%d", &i); // (2)
+
+ // The 'stdin' mutable system global variable gets invalidated by the first
+ // (1) opaque 'fscanf()' call. Consequently, the subsequent
+ // (2) 'fscanf(stdin, ...)' call won't propagate taint.
+ // FIXME: The analyzer engine should propagate taint by default on
+ // invalidation.
+ int j = i; // FIXME: should have 'tainted' warning here.
return m + j; // expected-warning + {{tainted}}
}
Index: clang/test/Analysis/Inputs/some_user_globals.h
===================================================================
--- /dev/null
+++ clang/test/Analysis/Inputs/some_user_globals.h
@@ -0,0 +1,2 @@
+extern const int my_const_user_global;
+extern int my_mutable_user_global;
Index: clang/test/Analysis/Inputs/some_system_globals.h
===================================================================
--- /dev/null
+++ clang/test/Analysis/Inputs/some_system_globals.h
@@ -0,0 +1,4 @@
+#pragma clang system_header
+
+extern const int my_const_system_global;
+extern int my_mutable_system_global;
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -973,26 +973,16 @@
const MemRegion *sReg = nullptr;
if (D->hasGlobalStorage() && !D->isStaticLocal()) {
-
- // First handle the globals defined in system headers.
- if (Ctx.getSourceManager().isInSystemHeader(D->getLocation())) {
- // Allow the system globals which often DO GET modified, assume the
- // rest are immutable.
- if (D->getName().contains("errno"))
- sReg = getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
- else
- sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
-
- // Treat other globals as GlobalInternal unless they are constants.
- } else {
- QualType GQT = D->getType();
- const Type *GT = GQT.getTypePtrOrNull();
+ QualType Ty = D->getType();
+ assert(!Ty.isNull());
+ if (Ty.isConstQualified() && Ty->isArithmeticType()) {
// TODO: We could walk the complex types here and see if everything is
// constified.
- if (GT && GQT.isConstQualified() && GT->isArithmeticType())
- sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
- else
- sReg = getGlobalsRegion();
+ sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+ } else if (Ctx.getSourceManager().isInSystemHeader(D->getLocation())) {
+ sReg = getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
+ } else {
+ sReg = getGlobalsRegion(MemRegion::GlobalInternalSpaceRegionKind);
}
// Finally handle static locals.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
