-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[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
Changes from 16 commits
0630d81
fa3f84f
b022182
5190ee0
856a865
d8f3456
01d0521
2aacf92
6e8a332
d65aa88
7e94b10
4db2804
7e73177
cac94fe
373679b
7cbabf8
e377b19
cfedf88
fa0a379
4e6c988
17a0e9c
89389a5
4d883f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// Global const variable | ||
const int global_const = 42; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 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 commentThe 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 commentThe 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. |
||
|
||
// Const struct member | ||
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 works in C++, but not in C, as the analyzer treats | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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". |
||
// 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 | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,168 @@ | ||||||||||||||||||
//=== 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; | ||||||||||||||||||
|
||||||||||||||||||
private: | ||||||||||||||||||
bool isInitializationContext(const Stmt *S, CheckerContext &C) const; | ||||||||||||||||||
bool isEffectivelyConstRegion(const MemRegion *MR, CheckerContext &C) const; | ||||||||||||||||||
}; | ||||||||||||||||||
} // end anonymous namespace | ||||||||||||||||||
|
||||||||||||||||||
bool StoreToImmutableChecker::isInitializationContext(const Stmt *S, | ||||||||||||||||||
CheckerContext &C) const { | ||||||||||||||||||
// 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 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 commentThe reason will be displayed to describe this comment to others. Learn more. Added this with a sidenote. |
||||||||||||||||||
const auto *ConstructExp = dyn_cast<CXXConstructExpr>(S); | ||||||||||||||||||
return ConstructExp && ConstructExp->isElidable(); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
static bool isEffectivelyConstRegionAux(const MemRegion *MR, | ||||||||||||||||||
CheckerContext &C) { | ||||||||||||||||||
// 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 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 only kind of region that is not checked by the above branches is | ||||||||||||||||||
// AllocaRegion. We do not need to check AllocaRegion, as it models untyped | ||||||||||||||||||
// memory, that is allocated on the stack. | ||||||||||||||||||
|
||||||||||||||||||
return false; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
bool StoreToImmutableChecker::isEffectivelyConstRegion( | ||||||||||||||||||
const MemRegion *MR, CheckerContext &C) const { | ||||||||||||||||||
NagyDonat marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
while (true) { | ||||||||||||||||||
if (isEffectivelyConstRegionAux(MR, C)) | ||||||||||||||||||
return true; | ||||||||||||||||||
if (auto *SR = dyn_cast<SubRegion>(MR)) | ||||||||||||||||||
MR = SR->getSuperRegion(); | ||||||||||||||||||
else | ||||||||||||||||||
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 | ||||||||||||||||||
// 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 corresponds to a const variable | ||||||||||||||||||
if (!isEffectivelyConstRegion(MR, C)) | ||||||||||||||||||
return; | ||||||||||||||||||
|
||||||||||||||||||
// Generate the bug report | ||||||||||||||||||
ExplodedNode *N = C.generateNonFatalErrorNode(); | ||||||||||||||||||
if (!N) | ||||||||||||||||||
return; | ||||||||||||||||||
|
||||||||||||||||||
constexpr llvm::StringLiteral Msg = "Trying to write to immutable memory."; | ||||||||||||||||||
|
||||||||||||||||||
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 declared as immutable here", | ||||||||||||||||||
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; | ||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
// 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}} | ||
} | ||
|
||
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}} | ||
} | ||
|
||
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}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I have added more elaborate note implementation and tests. |
||
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}; | ||
|
||
void test_write_to_const_global_array() { | ||
int *ptr = (int*)global_array; | ||
ptr[0] = 10; // expected-warning {{Trying to write to immutable memory}} | ||
} | ||
|
||
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}} | ||
} | ||
|
||
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}} | ||
} |
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.