[clang] f4fc3f6 - [analyzer] Treat system globals as mutable if they are not const
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 15 08:08:48 PDT 2022
Author: Balazs Benics
Date: 2022-06-15T17:08:27+02:00
New Revision: f4fc3f6ba319c3c571b6a713a1c38ca1e1e3aae5
URL: https://github.com/llvm/llvm-project/commit/f4fc3f6ba319c3c571b6a713a1c38ca1e1e3aae5
DIFF: https://github.com/llvm/llvm-project/commit/f4fc3f6ba319c3c571b6a713a1c38ca1e1e3aae5.diff
LOG: [analyzer] Treat system globals as mutable if they are not const
Previously, system globals were treated as immutable regions, unless it
was the `errno` which is known to be frequently modified.
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.
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.
---
In the tests I added, only the last `expected-warning` was different, compared to the baseline.
Which is this:
```lang=C++
void test_my_mutable_system_global_constraint() {
assert(my_mutable_system_global > 2);
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.
}
void test_my_mutable_system_global_assign(int x) {
my_mutable_system_global = x;
clang_analyzer_eval(my_mutable_system_global == x); // expected-warning {{TRUE}}
invalidate_globals();
clang_analyzer_eval(my_mutable_system_global == x); // expected-warning {{UNKNOWN}} It was previously TRUE.
}
```
---
Unfortunately, the taint checker will be also affected.
The `stdin` global variable is a pointer, which is assumed to be a taint
source, and the rest of the taint propagation rules will propagate from
it.
However, since mutable variables are no longer treated immutable, they
also get invalidated, when an opaque function call happens, such as the
first `scanf(stdin, ...)`. This would effectively remove taint from the
pointer, consequently disable all the rest of the taint propagations
down the line from the `stdin` variable.
All that said, I decided to look through `DerivedSymbol`s as well, to
acquire the memregion in that case as well. This should preserve the
previously existing taint reports.
Reviewed By: martong
Differential Revision: https://reviews.llvm.org/D127306
Added:
clang/test/Analysis/Inputs/some_system_globals.h
clang/test/Analysis/Inputs/some_user_globals.h
clang/test/Analysis/globals-are-not-always-immutable.c
Modified:
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index 567688b267d58..2c02ffe33dea6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -92,10 +92,8 @@ bool isStdin(SVal Val, const ASTContext &ACtx) {
return false;
// Get it's symbol and find the declaration region it's pointing to.
- const auto *Sm = dyn_cast<SymbolRegionValue>(SymReg->getSymbol());
- if (!Sm)
- return false;
- const auto *DeclReg = dyn_cast<DeclRegion>(Sm->getRegion());
+ const auto *DeclReg =
+ dyn_cast_or_null<DeclRegion>(SymReg->getSymbol()->getOriginRegion());
if (!DeclReg)
return false;
diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
index 7c9babf894c7b..96a985381acc8 100644
--- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -973,26 +973,16 @@ const VarRegion *MemRegionManager::getVarRegion(const VarDecl *D,
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.
diff --git a/clang/test/Analysis/Inputs/some_system_globals.h b/clang/test/Analysis/Inputs/some_system_globals.h
new file mode 100644
index 0000000000000..81d5ead82e074
--- /dev/null
+++ b/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;
diff --git a/clang/test/Analysis/Inputs/some_user_globals.h b/clang/test/Analysis/Inputs/some_user_globals.h
new file mode 100644
index 0000000000000..a63138a1a9d48
--- /dev/null
+++ b/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;
diff --git a/clang/test/Analysis/globals-are-not-always-immutable.c b/clang/test/Analysis/globals-are-not-always-immutable.c
new file mode 100644
index 0000000000000..26609095d8c50
--- /dev/null
+++ b/clang/test/Analysis/globals-are-not-always-immutable.c
@@ -0,0 +1,71 @@
+// 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"
+
+extern void abort() __attribute__((__noreturn__));
+#define assert(expr) ((expr) ? (void)(0) : abort())
+
+void invalidate_globals(void);
+void clang_analyzer_eval(int x);
+
+/// Test the special system 'errno'
+void test_errno_constraint() {
+ assert(errno > 2);
+ clang_analyzer_eval(errno > 2); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(errno > 2); // expected-warning {{UNKNOWN}}
+}
+void test_errno_assign(int x) {
+ errno = x;
+ clang_analyzer_eval(errno == x); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(errno == x); // expected-warning {{UNKNOWN}}
+}
+
+/// Test user global variables
+void test_my_const_user_global_constraint() {
+ assert(my_const_user_global > 2);
+ 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_const_user_global_assign(int); // One cannot assign value to a const lvalue.
+
+void test_my_mutable_user_global_constraint() {
+ assert(my_mutable_user_global > 2);
+ clang_analyzer_eval(my_mutable_user_global > 2); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(my_mutable_user_global > 2); // expected-warning {{UNKNOWN}}
+}
+void test_my_mutable_user_global_assign(int x) {
+ my_mutable_user_global = x;
+ clang_analyzer_eval(my_mutable_user_global == x); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(my_mutable_user_global == x); // expected-warning {{UNKNOWN}}
+}
+
+/// Test system global variables
+void test_my_const_system_global_constraint() {
+ assert(my_const_system_global > 2);
+ 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_const_system_global_assign(int);// One cannot assign value to a const lvalue.
+
+void test_my_mutable_system_global_constraint() {
+ assert(my_mutable_system_global > 2);
+ clang_analyzer_eval(my_mutable_system_global > 2); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(my_mutable_system_global > 2); // expected-warning {{UNKNOWN}}
+}
+void test_my_mutable_system_global_assign(int x) {
+ my_mutable_system_global = x;
+ clang_analyzer_eval(my_mutable_system_global == x); // expected-warning {{TRUE}}
+ invalidate_globals();
+ clang_analyzer_eval(my_mutable_system_global == x); // expected-warning {{UNKNOWN}}
+}
More information about the cfe-commits
mailing list