Skip to content

[clang][analyzer] Add notes to PointerSubChecker #95899

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.

Sign up for GitHub

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 4 commits into from
Jun 24, 2024
Merged

[clang][analyzer] Add notes to PointerSubChecker #95899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1eb6e7e
[clang][analyzer] Add notes to PointerSubChecker
balazske Jun 18, 2024
de36da1
check if the notes would appear at same location
balazske Jun 18, 2024
5f667f4
fixed failing tests
balazske Jun 19, 2024
b95cf7c
fixed review issues
balazske Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 36 additions & 15 deletions clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ class PointerSubChecker
bool checkArrayBounds(CheckerContext &C, const Expr *E,
const ElementRegion *ElemReg,
const MemRegion *Reg) const;
void reportBug(CheckerContext &C, const Expr *E,
const llvm::StringLiteral &Msg) const;

public:
void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
Expand All @@ -57,14 +55,22 @@ bool PointerSubChecker::checkArrayBounds(CheckerContext &C, const Expr *E,
if (!ElemReg)
return true;

auto ReportBug = [&](const llvm::StringLiteral &Msg) {
if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
R->addRange(E->getSourceRange());
C.emitReport(std::move(R));
}
};

ProgramStateRef State = C.getState();
const MemRegion *SuperReg = ElemReg->getSuperRegion();
SValBuilder &SVB = C.getSValBuilder();

if (SuperReg == Reg) {
if (const llvm::APSInt *I = SVB.getKnownValue(State, ElemReg->getIndex());
I && (!I->isOne() && !I->isZero()))
reportBug(C, E, Msg_BadVarIndex);
ReportBug(Msg_BadVarIndex);
return false;
}

Expand All @@ -77,7 +83,7 @@ bool PointerSubChecker::checkArrayBounds(CheckerContext &C, const Expr *E,
ProgramStateRef S1, S2;
std::tie(S1, S2) = C.getState()->assume(*IndexTooLarge);
if (S1 && !S2) {
reportBug(C, E, Msg_LargeArrayIndex);
ReportBug(Msg_LargeArrayIndex);
return false;
}
}
Expand All @@ -89,22 +95,13 @@ bool PointerSubChecker::checkArrayBounds(CheckerContext &C, const Expr *E,
ProgramStateRef S1, S2;
std::tie(S1, S2) = State->assume(*IndexTooSmall);
if (S1 && !S2) {
reportBug(C, E, Msg_NegativeArrayIndex);
ReportBug(Msg_NegativeArrayIndex);
return false;
}
}
return true;
}

void PointerSubChecker::reportBug(CheckerContext &C, const Expr *E,
const llvm::StringLiteral &Msg) const {
if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
R->addRange(E->getSourceRange());
C.emitReport(std::move(R));
}
}

void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
CheckerContext &C) const {
// When doing pointer subtraction, if the two pointers do not point to the
Expand Down Expand Up @@ -136,6 +133,9 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
if (!checkArrayBounds(C, B->getRHS(), ElemRR, LR))
return;

const ValueDecl *DiffDeclL = nullptr;
const ValueDecl *DiffDeclR = nullptr;

if (ElemLR && ElemRR) {
const MemRegion *SuperLR = ElemLR->getSuperRegion();
const MemRegion *SuperRR = ElemRR->getSuperRegion();
Expand All @@ -144,9 +144,30 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
// Allow arithmetic on different symbolic regions.
if (isa<SymbolicRegion>(SuperLR) || isa<SymbolicRegion>(SuperRR))
return;
if (const auto *SuperDLR = dyn_cast<DeclRegion>(SuperLR))
DiffDeclL = SuperDLR->getDecl();
if (const auto *SuperDRR = dyn_cast<DeclRegion>(SuperRR))
DiffDeclR = SuperDRR->getDecl();
Comment on lines +147 to +150
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that FieldRegions are DeclRegions, so these declarations may be data member declarations within a struct or class. This is usually not a problem, but there is a corner case where SuperLR != SuperRR, but the corresponding declarations are identical:

struct {
  int array[5];
} a, b;
int func(void) {
  return &a.array[3] - &b.array[2];
}

In this case the current code would place both notes onto the declaration of field, which would be confusing for the user.

Consider adding some code that handles this situation explicitly. (Either simply skip note creation when DiffDeclL == DiffDeclR, or create a specialized note for this case.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning is now omitted if both would be at the same declaration.

}

reportBug(C, B, Msg_MemRegionDifferent);
if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
auto R =
std::make_unique<PathSensitiveBugReport>(BT, Msg_MemRegionDifferent, N);
R->addRange(B->getSourceRange());
// The declarations may be identical even if the regions are different:
// struct { int array[10]; } a, b;
// do_something(&a.array[5] - &b.array[5]);
// In this case don't emit notes.
if (DiffDeclL != DiffDeclR) {
Copy link
Contributor

@NagyDonat NagyDonat Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (DiffDeclL != DiffDeclR) {
if (DiffDeclL != DiffDeclR) {
// The declarations may be identical even if the regions are different,
// if they are field regions within different objects:
// struct { int array[10]; } a, b;
// do_something_with(&a.array[5] - &b.array[5]);
// In this case the notes would be confusing, so don't emit them.

Add a comment that explains this if (DiffDeclL != DiffDeclR) check because otherwise it would be very surprising.

if (DiffDeclL)
R->addNote("Array at the left-hand side of subtraction",
{DiffDeclL, C.getSourceManager()});
if (DiffDeclR)
R->addNote("Array at the right-hand side of subtraction",
{DiffDeclR, C.getSourceManager()});
}
C.emitReport(std::move(R));
}
}

void ento::registerPointerSubChecker(CheckerManager &mgr) {
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/casts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ void locAsIntegerCasts(void *p) {
}

void multiDimensionalArrayPointerCasts(void) {
static int x[10][10];
static int x[10][10]; // expected-note2{{Array at the right-hand side of subtraction}}
int *y1 = &(x[3][5]);
char *z = ((char *) y1) + 2;
int *y2 = (int *)(z - 2);
Expand Down
66 changes: 66 additions & 0 deletions clang/test/Analysis/pointer-sub-notes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.PointerSub -analyzer-output=text -verify %s

void negative_1() {
int a[3];
int x = -1;
// FIXME: should indicate that 'x' is -1
int d = &a[x] - &a[0]; // expected-warning{{Using a negative array index at pointer subtraction is undefined behavior}} \
// expected-note{{Using a negative array index at pointer subtraction is undefined behavior}}
}

void negative_2() {
int a[3];
int *p1 = a, *p2 = a;
--p2;
// FIXME: should indicate that 'p2' is negative
int d = p1 - p2; // expected-warning{{Using a negative array index at pointer subtraction is undefined behavior}} \
// expected-note{{Using a negative array index at pointer subtraction is undefined behavior}}
}

void different_1() {
int a[3]; // expected-note{{Array at the left-hand side of subtraction}}
int b[3]; // expected-note{{Array at the right-hand side of subtraction}}
int d = &a[2] - &b[0]; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}} \
// expected-note{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
}

void different_2() {
int a[3]; // expected-note{{Array at the right-hand side of subtraction}}
int b[3]; // expected-note{{Array at the left-hand side of subtraction}}
int *p1 = a + 1;
int *p2 = b;
int d = p2 - p1; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}} \
// expected-note{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
}

int different_3() {
struct {
int array[5];
} a, b;
return &a.array[3] - &b.array[2]; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}} \
// expected-note{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
}

int different_4() {
struct {
int array1[5]; // expected-note{{Array at the left-hand side of subtraction}}
int array2[5]; // expected-note{{Array at the right-hand side of subtraction}}
} a;
return &a.array1[3] - &a.array2[4]; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}} \
// expected-note{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
}

void different_5() {
int d;
static int x[10][10]; // expected-note2{{Array at the left-hand side of subtraction}}
int *y1 = &(x[3][5]);
char *z = ((char *) y1) + 2;
int *y2 = (int *)(z - 2);
int *y3 = ((int *)x) + 35; // This is offset for [3][5].

d = y2 - y1; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}} \
// expected-note{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
d = y3 - y1; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}} \
// expected-note{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
d = y3 - y2;
}
14 changes: 8 additions & 6 deletions clang/test/Analysis/pointer-sub.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ void f1(void) {
}

void f2(void) {
int a[10], b[10], c;
int a[10], b[10], c; // expected-note{{Array at the left-hand side of subtraction}} \
// expected-note2{{Array at the right-hand side of subtraction}}
int *p = &a[2];
int *q = &a[8];
int d = q - p; // no-warning (pointers into the same array)
Expand All @@ -41,7 +42,8 @@ void f2(void) {
}

void f3(void) {
int a[3][4];
int a[3][4]; // expected-note{{Array at the left-hand side of subtraction}} \
// expected-note2{{Array at the right-hand side of subtraction}}
int d;

d = &(a[2]) - &(a[1]);
Expand Down Expand Up @@ -74,8 +76,8 @@ void f4(void) {
typedef struct {
int a;
int b;
int c[10];
int d[10];
int c[10]; // expected-note2{{Array at the right-hand side of subtraction}}
int d[10]; // expected-note2{{Array at the left-hand side of subtraction}}
} S;

void f5(void) {
Expand All @@ -100,8 +102,8 @@ void f6(void) {
char *a1 = (char *)&l;
int d = a1[3] - l;

long long la1[3];
long long la2[3];
long long la1[3]; // expected-note{{Array at the right-hand side of subtraction}}
long long la2[3]; // expected-note{{Array at the left-hand side of subtraction}}
char *pla1 = (char *)la1;
char *pla2 = (char *)la2;
d = pla1[1] - pla1[0];
Expand Down
Loading