Skip to content

[analyzer] Show element count in ArrayBound underflow reports #158639

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 5 commits into from
Sep 17, 2025
Merged

[analyzer] Show element count in ArrayBound underflow reports #158639

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db0723c
[analyzer] Show element count in ArrayBound underflow reports
NagyDonat Sep 15, 2025
78be939
Reimplement with different approach
NagyDonat Sep 16, 2025
51cac47
Satisfy git-clang-format
NagyDonat Sep 16, 2025
e82f75f
Make helper functions 'static'
NagyDonat Sep 16, 2025
5b1646f
Make 'ASTContext &' parameter const
NagyDonat Sep 16, 2025
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
48 changes: 31 additions & 17 deletions clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,15 +389,26 @@ static std::optional<int64_t> getConcreteValue(std::optional<NonLoc> SV) {
}

static Messages getPrecedesMsgs(const MemSpaceRegion *Space,
const SubRegion *Region, NonLoc Offset) {
std::string RegName = getRegionName(Space, Region), OffsetStr = "";
const SubRegion *Region, NonLoc Offset,
QualType ElemType, int64_t ElemSize) {
std::string RegName = getRegionName(Space, Region);

if (auto ConcreteOffset = getConcreteValue(Offset))
std::string OffsetStr = "", ElemInfoStr = "";
if (std::optional<int64_t> ConcreteOffset = getConcreteValue(Offset)) {
OffsetStr = formatv(" {0}", ConcreteOffset);
if (*ConcreteOffset % ElemSize == 0) {
int64_t Count = *ConcreteOffset / ElemSize;
if (Count != -1)
ElemInfoStr =
formatv(" = {0} * sizeof({1})", Count, ElemType.getAsString());
else
ElemInfoStr = formatv(" = -sizeof({0})", ElemType.getAsString());
}
}

return {
formatv("Out of bound access to memory preceding {0}", RegName),
formatv("Access of {0} at negative byte offset{1}", RegName, OffsetStr)};
return {formatv("Out of bound access to memory preceding {0}", RegName),
formatv("Access of {0} at negative byte offset{1}{2}", RegName,
OffsetStr, ElemInfoStr)};
}

/// Try to divide `Val1` and `Val2` (in place) by `Divisor` and return true if
Expand All @@ -419,20 +430,15 @@ static bool tryDividePair(std::optional<int64_t> &Val1,
return true;
}

static Messages getExceedsMsgs(ASTContext &ACtx, const MemSpaceRegion *Space,
static Messages getExceedsMsgs(const MemSpaceRegion *Space,
const SubRegion *Region, NonLoc Offset,
NonLoc Extent, SVal Location,
bool AlsoMentionUnderflow) {
NonLoc Extent, bool AlsoMentionUnderflow,
QualType ElemType, int64_t ElemSize) {
std::string RegName = getRegionName(Space, Region);
const auto *EReg = Location.getAsRegion()->getAs<ElementRegion>();
assert(EReg && "this checker only handles element access");
QualType ElemType = EReg->getElementType();

std::optional<int64_t> OffsetN = getConcreteValue(Offset);
std::optional<int64_t> ExtentN = getConcreteValue(Extent);

int64_t ElemSize = ACtx.getTypeSizeInChars(ElemType).getQuantity();

bool UseByteOffsets = !tryDividePair(OffsetN, ExtentN, ElemSize);
const char *OffsetOrIndex = UseByteOffsets ? "byte offset" : "index";

Expand Down Expand Up @@ -585,6 +591,13 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const {
if (!RawOffset)
return;

const auto *EReg = Location.getAsRegion()->getAs<ElementRegion>();
assert(EReg && "this checker only handles element access");
QualType ElemType = EReg->getElementType();

int64_t ElemSize =
C.getASTContext().getTypeSizeInChars(ElemType).getQuantity();

auto [Reg, ByteOffset] = *RawOffset;

// The state updates will be reported as a single note tag, which will be
Expand Down Expand Up @@ -635,7 +648,8 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const {
} else {
if (!WithinLowerBound) {
// ...and it cannot be valid (>= 0), so report an error.
Messages Msgs = getPrecedesMsgs(Space, Reg, ByteOffset);
Messages Msgs =
getPrecedesMsgs(Space, Reg, ByteOffset, ElemType, ElemSize);
reportOOB(C, PrecedesLowerBound, Msgs, ByteOffset, std::nullopt);
return;
}
Expand Down Expand Up @@ -678,8 +692,8 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const {
}

Messages Msgs =
getExceedsMsgs(C.getASTContext(), Space, Reg, ByteOffset,
*KnownSize, Location, AlsoMentionUnderflow);
getExceedsMsgs(Space, Reg, ByteOffset, *KnownSize,
AlsoMentionUnderflow, ElemType, ElemSize);
reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize);
return;
}
Expand Down
36 changes: 33 additions & 3 deletions clang/test/Analysis/ArrayBound/verbose-tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,47 @@ int TenElements[10];
void arrayUnderflow(void) {
TenElements[-3] = 5;
// expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}}
// expected-note@-2 {{Access of 'TenElements' at negative byte offset -12}}
// expected-note@-2 {{Access of 'TenElements' at negative byte offset -12 = -3 * sizeof(int)}}
}

int underflowWithDeref(void) {
int *p = TenElements;
--p;
return *p;
// expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}}
// expected-note@-2 {{Access of 'TenElements' at negative byte offset -4}}
// expected-note@-2 {{Access of 'TenElements' at negative byte offset -4 = -sizeof(int)}}
}

char underflowReportedAsChar(void) {
// The "= -... * sizeof(type)" part uses the type of the accessed element
// (here 'char'), not the type that appears in the declaration of the
// original array (which would be 'int').
return ((char *)TenElements)[-1];
// expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}}
// expected-note@-2 {{Access of 'TenElements' at negative byte offset -1 = -sizeof(char)}}
}

struct TwoInts {
int a, b;
};

struct TwoInts underflowReportedAsStruct(void) {
// Another case where the accessed type is used for reporting the offset.
return *(struct TwoInts*)(TenElements - 4);
// expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}}
// expected-note@-2 {{Access of 'TenElements' at negative byte offset -16 = -2 * sizeof(struct TwoInts)}}
}

struct TwoInts underflowOnlyByteOffset(void) {
// In this case the negative byte offset is not a multiple of the size of the
// accessed element, so the part "= -... * sizeof(type)" is omitted at the
// end of the message.
return *(struct TwoInts*)(TenElements - 3);
// expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}}
// expected-note-re@-2 {{Access of 'TenElements' at negative byte offset -12{{$}}}}
}


int rng(void);
int getIndex(void) {
switch (rng()) {
Expand All @@ -40,7 +70,7 @@ void gh86959(void) {
while (rng())
TenElements[getIndex()] = 10;
// expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}}
// expected-note@-2 {{Access of 'TenElements' at negative byte offset -688}}
// expected-note@-2 {{Access of 'TenElements' at negative byte offset -688 = -172 * sizeof(int)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Its surprising to read this, as -172 * sizeof should result in a huge unsigned value due to wrapping.
Maybe a -172nd 'int' element would read better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll rewrite it somehow.

By the way I really hate that C has "unsigned" numbers (i.e. raw binary values without sign handling logic) which are frequently used as if they were "non-negative" numbers (i.e. proper numbers with normal arithmetic that happen to be non-negative). In particular, these "promote to unsigned" rules are the most problematic footguns in this area – it is completely nonsense that e.g. -1 > FFFFFFFFULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up switching to a different approach that generalizes the function generating the overflow message and uses it to generate the underflow message as well. This ensures that the two kinds of messages are formatted in a consistent style.

}

int scanf(const char *restrict fmt, ...);
Expand Down
Loading