-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[clang][analyzer] Add StoreToImmutable checker #150417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This adds alpha.core.StoreToImmutable, a new alpha checker that detects writes to immutable memory regions, implementing part of SEI CERT Rule ENV30-C. The original proposal only handled global const variables, but this implementation extends it to detect writes to: - Global const variables - Local const variables - String literals - Const parameters and struct members - Const arrays and pointers to const data This checker is the continuation of the work started by @zukatsinadze. Discussion: https://reviews.llvm.org/D124244
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Endre Fülöp (gamesh411) ChangesThis adds alpha.core.StoreToImmutable, a new alpha checker that detects writes
This checker is the continuation of the work started by @zukatsinadze. Full diff: https://github.com/llvm/llvm-project/pull/150417.diff 7 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 26c5028e04955..a00e9d81ba208 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -3077,6 +3077,23 @@ Either the comparison is useless or there is division by zero.
if (x == 0) { } // warn
}
+.. _alpha-core-StoreToImmutable:
+
+alpha.core.StoreToImmutable (C, C++)
+""""""""""""""""""""""""""""""""""""
+Check for writes to immutable memory regions. This implements part of SEI CERT Rule ENV30-C.
+
+This checker detects attempts to write to memory regions that are marked as immutable,
+including const variables, string literals, and other const-qualified memory.
+
+.. literalinclude:: checkers/storetoimmutable_example.cpp
+ :language: cpp
+
+**Solution**
+
+Avoid writing to const-qualified memory regions. If you need to modify the data,
+remove the const qualifier from the original declaration or use a mutable copy.
+
alpha.cplusplus
^^^^^^^^^^^^^^^
diff --git a/clang/docs/analyzer/checkers/storetoimmutable_example.cpp b/clang/docs/analyzer/checkers/storetoimmutable_example.cpp
new file mode 100644
index 0000000000000..e1a0683ff91e4
--- /dev/null
+++ b/clang/docs/analyzer/checkers/storetoimmutable_example.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.StoreToImmutable %s
+
+// Global const variable
+const int global_const = 42;
+
+void test_global_const() {
+ *(int *)&global_const = 100; // warn: Writing to immutable memory
+}
+
+// String literal
+void test_string_literal() {
+ char *str = (char *)"hello";
+ str[0] = 'H'; // warn: Writing to immutable memory
+}
+
+// Const parameter
+void test_const_param(const int param) {
+ *(int *)¶m = 100; // warn: Writing to immutable memory
+}
+
+// Const struct member
+struct TestStruct {
+ const int x;
+ int y;
+};
+
+void test_const_member() {
+ TestStruct s = {1, 2};
+ *(int *)&s.x = 10; // warn: Writing to immutable memory
+}
\ No newline at end of file
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 2234143004b6f..8799f1d15b0b6 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -306,6 +306,11 @@ def StackAddrAsyncEscapeChecker : Checker<"StackAddressAsyncEscape">,
Dependencies<[StackAddrEscapeBase]>,
Documentation<HasDocumentation>;
+def StoreToImmutableChecker : Checker<"StoreToImmutable">,
+ HelpText<"Check for writes to immutable memory regions. "
+ "This implements part of SEI CERT Rule ENV30-C.">,
+ Documentation<HasDocumentation>;
+
def PthreadLockBase : Checker<"PthreadLockBase">,
HelpText<"Helper registering multiple checks.">,
Documentation<NotDocumented>,
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
index 89d306fb94046..0f7bab6839703 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -819,7 +819,7 @@ class SymbolicRegion : public SubRegion {
s->getType()->isReferenceType() ||
s->getType()->isBlockPointerType());
assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg) ||
- isa<GlobalSystemSpaceRegion>(sreg));
+ isa<GlobalSystemSpaceRegion>(sreg) || isa<GlobalImmutableSpaceRegion>(sreg));
}
public:
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 22dd3f0374849..78360418a8b81 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -104,6 +104,7 @@ add_clang_library(clangStaticAnalyzerCheckers
SmartPtrChecker.cpp
SmartPtrModeling.cpp
StackAddrEscapeChecker.cpp
+ StoreToImmutableChecker.cpp
StdLibraryFunctionsChecker.cpp
StdVariantChecker.cpp
STLAlgorithmModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
new file mode 100644
index 0000000000000..a853e2e3f9c4a
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
@@ -0,0 +1,146 @@
+//=== StoreToImmutableChecker.cpp - Store to immutable memory checker -*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines StoreToImmutableChecker, a checker that detects writes
+// to immutable memory regions. This implements part of SEI CERT Rule ENV30-C.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class StoreToImmutableChecker : public Checker<check::Bind> {
+ const BugType BT{this, "Write to immutable memory", "CERT Environment (ENV)"};
+
+public:
+ void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const;
+
+private:
+ bool isConstVariable(const MemRegion *MR, CheckerContext &C) const;
+ bool isConstQualifiedType(const MemRegion *MR, CheckerContext &C) const;
+};
+} // end anonymous namespace
+
+bool StoreToImmutableChecker::isConstVariable(const MemRegion *MR,
+ CheckerContext &C) const {
+ // Check if the region is in the global immutable space
+ const MemSpaceRegion *MS = MR->getMemorySpace(C.getState());
+ if (isa<GlobalImmutableSpaceRegion>(MS))
+ return true;
+
+ // Check if this is a VarRegion with a const-qualified type
+ if (const VarRegion *VR = dyn_cast<VarRegion>(MR)) {
+ const VarDecl *VD = VR->getDecl();
+ if (VD && VD->getType().isConstQualified())
+ return true;
+ }
+
+ // Check if this is a ParamVarRegion with a const-qualified type
+ if (const ParamVarRegion *PVR = dyn_cast<ParamVarRegion>(MR)) {
+ const ParmVarDecl *PVD = PVR->getDecl();
+ if (PVD && PVD->getType().isConstQualified())
+ return true;
+ }
+
+ // Check if this is a FieldRegion with a const-qualified type
+ if (const FieldRegion *FR = dyn_cast<FieldRegion>(MR)) {
+ const FieldDecl *FD = FR->getDecl();
+ if (FD && FD->getType().isConstQualified())
+ return true;
+ }
+
+ // Check if this is a StringRegion (string literals are const)
+ if (isa<StringRegion>(MR))
+ return true;
+
+ // Check if this is a SymbolicRegion with a const-qualified pointee type
+ if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(MR)) {
+ QualType PointeeType = SR->getPointeeStaticType();
+ if (PointeeType.isConstQualified())
+ return true;
+ }
+
+ // Check if this is an ElementRegion accessing a const array
+ if (const ElementRegion *ER = dyn_cast<ElementRegion>(MR)) {
+ return isConstQualifiedType(ER->getSuperRegion(), C);
+ }
+
+ return false;
+}
+
+bool StoreToImmutableChecker::isConstQualifiedType(const MemRegion *MR,
+ CheckerContext &C) const {
+ // Check if the region has a const-qualified type
+ if (const TypedValueRegion *TVR = dyn_cast<TypedValueRegion>(MR)) {
+ QualType Ty = TVR->getValueType();
+ return Ty.isConstQualified();
+ }
+ return false;
+}
+
+void StoreToImmutableChecker::checkBind(SVal Loc, SVal Val, const Stmt *S,
+ CheckerContext &C) const {
+ // We are only interested in stores to memory regions
+ const MemRegion *MR = Loc.getAsRegion();
+ if (!MR)
+ return;
+
+ // Skip variable declarations and initializations - we only want to catch
+ // actual writes
+ if (isa<DeclStmt>(S) || isa<DeclRefExpr>(S))
+ return;
+
+ // Check if the region corresponds to a const variable
+ if (!isConstVariable(MR, C))
+ return;
+
+ const SourceManager &SM = C.getSourceManager();
+ // Skip if this is a system macro (likely from system headers)
+ if (SM.isInSystemMacro(S->getBeginLoc()))
+ return;
+
+ // Generate the bug report
+ ExplodedNode *N = C.generateNonFatalErrorNode();
+ if (!N)
+ return;
+
+ constexpr llvm::StringLiteral Msg =
+ "Writing to immutable memory is undefined behavior. "
+ "This memory region is marked as immutable and should not be modified.";
+
+ auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+ R->addRange(S->getSourceRange());
+
+ // If the location that is being written to has a declaration, place a note.
+ if (const DeclRegion *DR = dyn_cast<DeclRegion>(MR)) {
+ R->addNote("Memory region is in immutable space",
+ PathDiagnosticLocation::create(DR->getDecl(), SM));
+ }
+
+ // For this checker, we are only interested in the value being written, no
+ // need to mark the value being assigned interesting.
+
+ C.emitReport(std::move(R));
+}
+
+void ento::registerStoreToImmutableChecker(CheckerManager &mgr) {
+ mgr.registerChecker<StoreToImmutableChecker>();
+}
+
+bool ento::shouldRegisterStoreToImmutableChecker(const CheckerManager &mgr) {
+ return true;
+}
\ No newline at end of file
diff --git a/clang/test/Analysis/store-to-immutable-basic.cpp b/clang/test/Analysis/store-to-immutable-basic.cpp
new file mode 100644
index 0000000000000..2b28b933e97b4
--- /dev/null
+++ b/clang/test/Analysis/store-to-immutable-basic.cpp
@@ -0,0 +1,166 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.StoreToImmutable -verify %s
+
+// Test basic functionality of StoreToImmutable checker
+// This tests direct writes to immutable regions without function modeling
+
+// Direct write to a const global variable
+const int global_const = 42; // expected-note {{Memory region is in immutable space}}
+
+void test_direct_write_to_const_global() {
+ // This should trigger a warning about writing to immutable memory
+ *(int*)&global_const = 100; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write through a pointer to const memory
+void test_write_through_const_pointer() {
+ const int local_const = 10; // expected-note {{Memory region is in immutable space}}
+ int *ptr = (int*)&local_const;
+ *ptr = 20; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to string literal (should be in immutable space)
+void test_write_to_string_literal() {
+ char *str = (char*)"hello";
+ str[0] = 'H'; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const array
+void test_write_to_const_array() {
+ const int arr[5] = {1, 2, 3, 4, 5};
+ int *ptr = (int*)arr;
+ ptr[0] = 10; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const struct member
+struct TestStruct {
+ const int x; // expected-note 2{{Memory region is in immutable space}}
+ int y;
+};
+
+void test_write_to_const_struct_member() {
+ TestStruct s = {1, 2};
+ int *ptr = (int*)&s.x;
+ *ptr = 10; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const global array
+const int global_array[3] = {1, 2, 3};
+
+void test_write_to_const_global_array() {
+ int *ptr = (int*)global_array;
+ ptr[0] = 10; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const global struct
+const TestStruct global_struct = {1, 2};
+
+void test_write_to_const_global_struct() {
+ int *ptr = (int*)&global_struct.x;
+ *ptr = 10; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const parameter
+void test_write_to_const_param(const int param) { // expected-note {{Memory region is in immutable space}}
+ *(int*)¶m = 100; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const reference parameter
+void test_write_to_const_ref_param(const int ¶m) {
+ *(int*)¶m = 100; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const pointer parameter
+void test_write_to_const_ptr_param(const int *param) {
+ *(int*)param = 100; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const array parameter
+void test_write_to_const_array_param(const int arr[5]) {
+ *(int*)arr = 100; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const struct parameter
+struct ParamStruct {
+ const int z; // expected-note 3{{Memory region is in immutable space}}
+ int w;
+};
+
+void test_write_to_const_struct_param(const ParamStruct s) {
+ *(int*)&s.z = 100; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const struct reference parameter
+void test_write_to_const_struct_ref_param(const ParamStruct &s) {
+ *(int*)&s.z = 100; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+// Write to const struct pointer parameter
+void test_write_to_const_struct_ptr_param(const ParamStruct *s) {
+ *(int*)&s->z = 100; // expected-warning {{Writing to immutable memory is undefined behavior}}
+ // expected-note@-1 {{Writing to immutable memory is undefined behavior. This memory region is marked as immutable and should not be modified}}
+}
+
+//===--- NEGATIVE TEST CASES ---===//
+// These tests should NOT trigger warnings
+
+// Write to non-const variable (should not warn)
+void test_write_to_nonconst() {
+ int non_const = 42;
+ *(int*)&non_const = 100; // No warning expected
+}
+
+// Write to non-const global variable (should not warn)
+int global_non_const = 42;
+
+void test_write_to_nonconst_global() {
+ *(int*)&global_non_const = 100; // No warning expected
+}
+
+// Write to non-const struct member (should not warn)
+struct NonConstStruct {
+ int x;
+ int y;
+};
+
+void test_write_to_nonconst_struct_member() {
+ NonConstStruct s = {1, 2};
+ *(int*)&s.x = 100; // No warning expected
+}
+
+// Write to non-const parameter (should not warn)
+void test_write_to_nonconst_param(int param) {
+ *(int*)¶m = 100; // No warning expected
+}
+
+// Normal assignment to non-const variable (should not warn)
+void test_normal_assignment() {
+ int x = 42;
+ x = 100; // No warning expected
+}
+
+// Write to non-const data through const pointer (should not warn - underlying memory is non-const)
+void test_const_ptr_to_nonconst_data() {
+ int data = 42;
+ const int *ptr = &data;
+ *(int*)ptr = 100; // No warning expected
+}
+
+// Write to non-const data through const reference (should not warn - underlying memory is non-const)
+void test_const_ref_to_nonconst_data() {
+ int data = 42;
+ const int &ref = data;
+ *(int*)&ref = 100; // No warning expected
+}
\ No newline at end of file
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. It looks pretty solid.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
Outdated
Show resolved
Hide resolved
FYI please never tag anyone in commit title/message. And make sure that holds even if you push the Merge button on the GitHub UI. Otherwise the person would get pinged and spammed by emails forever. EDIT: I already edited the summary to not tag zukatsinadze if we ever merge this. |
add fixme comments add const reference test case remove duplicate expected-notes and analyzer-output=text
introduced an auxiliary function for checking non-ElementRegions we are now checking ElementRegions by walking up the superregion-chain a better name IMO for this function is isEffectivelyConstRegion
…d below these test cases assert the correct behavior, however without a fix for detecting that we are currently initializing a lambda, these fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for sticking around. I think we objectively made this better.
I'm sure both of us learned something along the way.
Please merge this with the final recommendations in place. Thank you!
I realize that it only took 5 days to iron everything out. It felt longer, but in then we made it. |
this cornercase has a test case in a previous commit also with the tag [cornercase]
@steakhal Thank you for your absolutely awesome work on this! I had a great time learning new stuff along the way! I have encountered a weird corner case that leads to a false positive. The issue appears in C++14 and before (does not occur in C++17 or after) when a const lambda is initialised. IMO, we can live with this hacky suppression of a false positive, and this breaks some unrelated test case in the build bots, so I cannot merge without at least this "quick-fix". |
I think it could be useful exposing if this bind is the initializing bind or not. So this case must something to do with mandatory copy elision or some sort mandated by cpp17? |
I just wanted to acknowledge that not all of our interactions went smooth in the past and some of your patches were halted, which is a frustrating feeling on both sides, and now I'm again, nitpicking your PR into pieces. Or at least, one might see this personal, so I wanted to make sure this time I'll follow more closely the development here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review suggestions resulting from consulting with @NagyDonat
} | ||
|
||
void test_write_to_const_array() { | ||
const int arr[5] = {1, 2, 3, 4, 5}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could give a note to the enclosing array declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently working on a better note placement logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added more elaborate note implementation and tests.
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.StoreToImmutable %s | ||
|
||
// Global const variable | ||
const int global_const = 42; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't emphasise "global" in variable names, as globals and local variables are handled the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true. Globals live in the global memory space where we have a suitable sub-space for immutable globals, like constants.
In contrast to this, in the stack space (where locals live) has no sub-space for constants. So there is an important semantic difference even for your checker because you also check the mspaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steakhal You are right that globals and locals are handled differently by the checker implementation, what we meant under "handled the same way" is the user experience. This is an example file that will be included in the documentation so we shouldn't highlight the global/local nature of variables if the user will see the same behavior (modifying const
values or parts of them is reported) in either case.
Alternatively including a global and a local variable to show that both produce the same behavior is also OK (I feel that it's a bit too verbose, but I'm not opposed to it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I misinterpreted the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the simplification, and it still has an example for a global and a local variable, demonstrating that misuses of both are detected.
removed runline collapsed the functions into a single one for brevity
change warning and note message simplify cornercase detection
@@ -0,0 +1,23 @@ | |||
// Global const variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer removing this comment and "// Const struct member" because they just state the obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed redundant comments from the example.
// FIXME: This should be eliminated once the API of checkBind would allow to | ||
// distinguish between initialization and assignment, because this information | ||
// is already available in the engine, it is just not passed to the checker | ||
// API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// FIXME: This should be eliminated once the API of checkBind would allow to | |
// distinguish between initialization and assignment, because this information | |
// is already available in the engine, it is just not passed to the checker | |
// API. | |
// FIXME: This should be eliminated by improving the API of checkBind to | |
// ensure that it consistently passes the `VarDecl` (instead of the | |
// `CXXConstructExpr`) when the constructor call denotes the initialization | |
// of a variable with a lambda. |
As we discussed in person, the most probable solution to this corner case is slightly different from what you originally wrote here. I hope that this will become irrelevant soon (if you can restore consistency in the engine), but it is still slightly better to document this more accurately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this with a sidenote.
Now we mention in the bug message whether the target is a global immutable memory, and thus the write is "fundamentally" bad, or it is just touching a const qualified type (still bad, but for another reason). Also added notes where the MemRegion being written to does not have a declaration. We now try to get the immediate cause of why the memory should not be written, and we put a note there. As a minor improvement, moved non-static member functions to free functions wherever possible, we are not using anything from the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with one very minor nitpick in a comment.
const int local_const = 42; | ||
*(int *)&local_const = 43; // warn: Trying to write to immutable memory | ||
|
||
// NOTE: The following works in C++, but not in C, as the analyzer treats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps replace "works" with "is reported" because "works" can be misunderstood as "is standard-compliant".
feda78b
to
4d883f1
Compare
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/21064 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/21171 Here is the relevant piece of the build log for the reference
|
This adds alpha.core.StoreToImmutable, a new alpha checker that detects writes
to immutable memory regions, implementing part of SEI CERT Rule ENV30-C. The
original proposal only handled global const variables, but this implementation
extends it to also detect writes to:
This checker is the continuation of the work started by zukatsinadze.
Discussion: https://reviews.llvm.org/D124244