-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[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
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
0630d81
[clang][analyzer] Add StoreToImmutable checker
gamesh411 fa3f84f
Apply review suggestiongs by @steakhal
gamesh411 b022182
[review-fix] fix test files
gamesh411 5190ee0
[review-fix] add test case for complex memory hierarchy
gamesh411 856a865
[review-fix] remove isInSystemMacro check
gamesh411 d8f3456
[review-fix] add example note on string literal limitation
gamesh411 01d0521
[review-fix] implement hierarchical memregion handling
gamesh411 2aacf92
[cornercase] Lambda initialization gives a false positive in C++14 an…
gamesh411 6e8a332
[format] fixed example file code formatting
gamesh411 d65aa88
[review-fix] don't repeat type names
gamesh411 7e94b10
[cornercase] fix false positive cornercase
gamesh411 4db2804
[review-fix] streamline example file
gamesh411 7e73177
[review-fix] add more C++ standard versions
gamesh411 cac94fe
[review-fix] streamline implementation
gamesh411 373679b
[review-fix] support SubRegions not just ElementRegions
gamesh411 7cbabf8
[review-fix] fix typo
gamesh411 e377b19
[review-fix] delete stray whitespace
gamesh411 cfedf88
[review-fix] more elaborate notes
gamesh411 fa0a379
[review-fix] remove redundant comments from example
gamesh411 4e6c988
[review-fix] document our options for the fixme
gamesh411 17a0e9c
Merge branch 'main' into store-to-immutable-checker
gamesh411 89389a5
[review-fix] clarify wording
gamesh411 4d883f1
fix formatting
gamesh411 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| const int global_const = 42; | ||
|
|
||
| struct TestStruct { | ||
| const int x; | ||
| int y; | ||
| }; | ||
|
|
||
| void immutable_violation_examples() { | ||
| *(int *)&global_const = 100; // warn: Trying to write to immutable memory | ||
|
|
||
| const int local_const = 42; | ||
| *(int *)&local_const = 43; // warn: Trying to write to immutable memory | ||
|
|
||
| // NOTE: The following is reported in C++, but not in C, as the analyzer | ||
| // treats string literals as non-const char arrays in C mode. | ||
| char *ptr_to_str_literal = (char *)"hello"; | ||
| ptr_to_str_literal[0] = 'H'; // warn: Trying to write to immutable memory | ||
|
|
||
| TestStruct s = {1, 2}; | ||
| *(int *)&s.x = 10; // warn: Trying to write to immutable memory | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
188 changes: 188 additions & 0 deletions
188
clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| //=== StoreToImmutableChecker.cpp - Store to immutable memory ---*- 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; | ||
| }; | ||
| } // end anonymous namespace | ||
|
|
||
| static bool isInitializationContext(const Stmt *S, CheckerContext &C) { | ||
| // Check if this is a DeclStmt (variable declaration) | ||
| if (isa<DeclStmt>(S)) | ||
| return true; | ||
|
|
||
| // This part is specific for initialization of const lambdas pre-C++17. | ||
| // Lets look at the AST of the statement: | ||
| // ``` | ||
| // const auto lambda = [](){}; | ||
| // ``` | ||
| // | ||
| // The relevant part of the AST for this case prior to C++17 is: | ||
| // ... | ||
| // `-DeclStmt | ||
| // `-VarDecl | ||
| // `-ExprWithCleanups | ||
| // `-CXXConstructExpr | ||
| // ... | ||
| // In C++17 and later, the AST is different: | ||
| // ... | ||
| // `-DeclStmt | ||
| // `-VarDecl | ||
| // `-ImplicitCastExpr | ||
| // `-LambdaExpr | ||
| // |-CXXRecordDecl | ||
| // `-CXXConstructExpr | ||
| // ... | ||
| // And even beside this, the statement `S` that is given to the checkBind | ||
| // callback is the VarDecl in C++17 and later, and the CXXConstructExpr in | ||
| // C++14 and before. So in order to support the C++14 we need the following | ||
| // ugly hack to detect whether this construction is used to initialize a | ||
| // variable. | ||
| // | ||
| // 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, or maybe less preferably, try the more | ||
| // invasive approach of passing the information forward to the checkers | ||
| // whether the current bind is an initialization or an assignment. | ||
| const auto *ConstructExp = dyn_cast<CXXConstructExpr>(S); | ||
| return ConstructExp && ConstructExp->isElidable(); | ||
| } | ||
|
|
||
| static bool isEffectivelyConstRegion(const MemRegion *MR, CheckerContext &C) { | ||
| if (isa<GlobalImmutableSpaceRegion>(MR)) | ||
| return true; | ||
|
|
||
| // Check if this is a TypedRegion with a const-qualified type | ||
| if (const auto *TR = dyn_cast<TypedRegion>(MR)) { | ||
| QualType LocationType = TR->getDesugaredLocationType(C.getASTContext()); | ||
| if (LocationType->isPointerOrReferenceType()) | ||
| LocationType = LocationType->getPointeeType(); | ||
| if (LocationType.isConstQualified()) | ||
| return true; | ||
| } | ||
|
|
||
| // Check if this is a SymbolicRegion with a const-qualified pointee type | ||
| if (const auto *SR = dyn_cast<SymbolicRegion>(MR)) { | ||
| QualType PointeeType = SR->getPointeeStaticType(); | ||
| if (PointeeType.isConstQualified()) | ||
| return true; | ||
| } | ||
|
|
||
| // NOTE: The above branches do not cover AllocaRegion. We do not need to check | ||
| // AllocaRegion, as it models untyped memory, that is allocated on the stack. | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| static const MemRegion *getInnermostConstRegion(const MemRegion *MR, | ||
| CheckerContext &C) { | ||
| while (true) { | ||
| if (isEffectivelyConstRegion(MR, C)) | ||
| return MR; | ||
| if (auto *SR = dyn_cast<SubRegion>(MR)) | ||
| MR = SR->getSuperRegion(); | ||
| else | ||
| return nullptr; | ||
| } | ||
| } | ||
|
|
||
| static const DeclRegion * | ||
| getInnermostEnclosingConstDeclRegion(const MemRegion *MR, CheckerContext &C) { | ||
| while (true) { | ||
| if (const auto *DR = dyn_cast<DeclRegion>(MR)) { | ||
| const ValueDecl *D = DR->getDecl(); | ||
| QualType DeclaredType = D->getType(); | ||
| if (DeclaredType.isConstQualified()) | ||
| return DR; | ||
| } | ||
| if (auto *SR = dyn_cast<SubRegion>(MR)) | ||
| MR = SR->getSuperRegion(); | ||
| else | ||
| return nullptr; | ||
| } | ||
| } | ||
|
|
||
| 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 | ||
| // FIXME: If the API of checkBind would allow to distinguish between | ||
| // initialization and assignment, we could use that instead. | ||
| if (isInitializationContext(S, C)) | ||
| return; | ||
|
|
||
| // Check if the region is in the global immutable space | ||
| const MemSpaceRegion *MS = MR->getMemorySpace(C.getState()); | ||
| const bool IsGlobalImmutableSpace = isa<GlobalImmutableSpaceRegion>(MS); | ||
| // Check if the region corresponds to a const variable | ||
| const MemRegion *InnermostConstRegion = getInnermostConstRegion(MR, C); | ||
| if (!IsGlobalImmutableSpace && !InnermostConstRegion) | ||
| return; | ||
|
|
||
| SmallString<64> WarningMessage{"Trying to write to immutable memory"}; | ||
| if (IsGlobalImmutableSpace) | ||
| WarningMessage += " in global read-only storage"; | ||
|
|
||
| // Generate the bug report | ||
| ExplodedNode *N = C.generateNonFatalErrorNode(); | ||
| if (!N) | ||
| return; | ||
|
|
||
| auto R = std::make_unique<PathSensitiveBugReport>(BT, WarningMessage, N); | ||
| R->addRange(S->getSourceRange()); | ||
|
|
||
| // Generate a note if the location that is being written to has a | ||
| // declaration or if it is a subregion of a const region with a declaration. | ||
| const DeclRegion *DR = | ||
| getInnermostEnclosingConstDeclRegion(InnermostConstRegion, C); | ||
| if (DR) { | ||
| const char *NoteMessage = | ||
| (DR != MR) ? "Enclosing memory region is declared as immutable here" | ||
| : "Memory region is declared as immutable here"; | ||
| R->addNote(NoteMessage, PathDiagnosticLocation::create( | ||
| DR->getDecl(), C.getSourceManager())); | ||
| } | ||
|
|
||
| // 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; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.StoreToImmutable -verify %s | ||
|
|
||
| // Test basic functionality of StoreToImmutable checker for the C programming language. | ||
|
|
||
| const int tentative_global_const; // expected-note {{Memory region is declared as immutable here}} | ||
|
|
||
| void test_direct_write_to_tentative_const_global() { | ||
| *(int*)&tentative_global_const = 100; // expected-warning {{Trying to write to immutable memory in global read-only storage}} | ||
| } | ||
|
|
||
| const int global_const = 42; // expected-note {{Memory region is declared as immutable here}} | ||
|
|
||
| void test_direct_write_to_const_global() { | ||
| // This should trigger a warning about writing to immutable memory | ||
| *(int*)&global_const = 100; // expected-warning {{Trying to write to immutable memory in global read-only storage}} | ||
| } | ||
|
|
||
| void test_write_through_const_pointer() { | ||
| const int local_const = 10; // expected-note {{Memory region is declared as immutable here}} | ||
| int *ptr = (int*)&local_const; | ||
| *ptr = 20; // expected-warning {{Trying to write to immutable memory}} | ||
| } | ||
|
|
||
| void test_write_to_const_array() { | ||
| const int arr[5] = {1, 2, 3, 4, 5}; // expected-note {{Enclosing memory region is declared as immutable here}} | ||
| int *ptr = (int*)arr; | ||
| ptr[0] = 10; // expected-warning {{Trying to write to immutable memory}} | ||
| } | ||
|
|
||
| struct TestStruct { | ||
| const int x; // expected-note 2 {{Memory region is declared as immutable here}} | ||
| int y; | ||
| }; | ||
|
|
||
| void test_write_to_const_struct_member() { | ||
| struct TestStruct s = {1, 2}; | ||
| int *ptr = (int*)&s.x; | ||
| *ptr = 10; // expected-warning {{Trying to write to immutable memory}} | ||
| } | ||
|
|
||
| const int global_array[3] = {1, 2, 3}; // expected-note {{Enclosing memory region is declared as immutable here}} | ||
|
|
||
| void test_write_to_const_global_array() { | ||
| int *ptr = (int*)global_array; | ||
| ptr[0] = 10; // expected-warning {{Trying to write to immutable memory in global read-only storage}} | ||
| } | ||
|
|
||
| const struct TestStruct global_struct = {1, 2}; | ||
|
|
||
| void test_write_to_const_global_struct() { | ||
| int *ptr = (int*)&global_struct.x; | ||
| *ptr = 10; // expected-warning {{Trying to write to immutable memory in global read-only storage}} | ||
| } | ||
|
|
||
|
|
||
| void test_write_to_const_param(const int param) { // expected-note {{Memory region is declared as immutable here}} | ||
| *(int*)¶m = 100; // expected-warning {{Trying to write to immutable memory}} | ||
| } | ||
|
|
||
| void test_write_to_const_ptr_param(const int *param) { | ||
| *(int*)param = 100; // expected-warning {{Trying to write to immutable memory}} | ||
| } | ||
|
|
||
| void test_write_to_const_array_param(const int arr[5]) { | ||
| *(int*)arr = 100; // expected-warning {{Trying to write to immutable memory}} | ||
| } | ||
|
|
||
| struct ParamStruct { | ||
| const int z; // expected-note 2 {{Memory region is declared as immutable here}} | ||
| int w; | ||
| }; | ||
|
|
||
| void test_write_to_const_struct_param(const struct ParamStruct s) { | ||
| *(int*)&s.z = 100; // expected-warning {{Trying to write to immutable memory}} | ||
| } | ||
|
|
||
| void test_write_to_const_struct_ptr_param(const struct ParamStruct *s) { | ||
| *(int*)&s->z = 100; // expected-warning {{Trying to write to immutable memory}} | ||
| } | ||
|
|
||
| void test_write_to_nonconst() { | ||
| int non_const = 42; | ||
| *(int*)&non_const = 100; // No warning expected | ||
| } | ||
|
|
||
| int global_non_const = 42; | ||
|
|
||
| void test_write_to_nonconst_global() { | ||
| *(int*)&global_non_const = 100; // No warning expected | ||
| } | ||
|
|
||
| struct NonConstStruct { | ||
| int x; | ||
| int y; | ||
| }; | ||
|
|
||
| void test_write_to_nonconst_struct_member() { | ||
| struct NonConstStruct s = {1, 2}; | ||
| *(int*)&s.x = 100; // No warning expected | ||
| } | ||
|
|
||
| void test_write_to_nonconst_param(int param) { | ||
| *(int*)¶m = 100; // No warning expected | ||
| } | ||
|
|
||
| void test_normal_assignment() { | ||
| int x = 42; | ||
| x = 100; // No warning expected | ||
| } | ||
|
|
||
| void test_const_ptr_to_nonconst_data() { | ||
| int data = 42; | ||
| const int *ptr = &data; | ||
| *(int*)ptr = 100; // No warning expected | ||
| } | ||
|
|
||
| void test_const_ptr_to_const_data() { | ||
| const int data = 42; // expected-note {{Memory region is declared as immutable here}} | ||
| const int *ptr = &data; | ||
| *(int*)ptr = 100; // expected-warning {{Trying to write to immutable memory}} | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Uh oh!
There was an error while loading. Please reload this page.
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
constvalues 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.