Skip to content

Commit 0954dc3

Browse files
committed
[analyzer] CStringChecker buffer access checks should check the first bytes
By not checking if the first byte of the buffer is accessible, we missed some reports in the Juliet benchmark. (Juliet CWE-124 Buffer Underwrite: memcpy, memmove) https://discourse.llvm.org/t/patches-inspired-by-the-juliet-benchmark/73106 Depends on D159108 Differential Revision: https://reviews.llvm.org/D159109
1 parent c3a87dd commit 0954dc3

File tree

2 files changed

+27
-2
lines changed

2 files changed

+27
-2
lines changed

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,14 @@ CStringChecker::CheckBufferAccess(CheckerContext &C, ProgramStateRef State,
480480
if (!Filter.CheckCStringOutOfBounds)
481481
return State;
482482

483+
SVal BufStart =
484+
svalBuilder.evalCast(BufVal, PtrTy, Buffer.Expression->getType());
485+
486+
// Check if the first byte of the buffer is accessible.
487+
State = CheckLocation(C, State, Buffer, BufStart, Access, CK);
488+
if (!State)
489+
return nullptr;
490+
483491
// Get the access length and make sure it is known.
484492
// FIXME: This assumes the caller has already checked that the access length
485493
// is positive. And that it's unsigned.
@@ -496,8 +504,6 @@ CStringChecker::CheckBufferAccess(CheckerContext &C, ProgramStateRef State,
496504
NonLoc LastOffset = Offset.castAs<NonLoc>();
497505

498506
// Check that the first buffer is sufficiently long.
499-
SVal BufStart =
500-
svalBuilder.evalCast(BufVal, PtrTy, Buffer.Expression->getType());
501507
if (std::optional<Loc> BufLoc = BufStart.getAs<Loc>()) {
502508

503509
SVal BufEnd =

clang/test/Analysis/string.c

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ void clang_analyzer_eval(int);
7171
int scanf(const char *restrict format, ...);
7272
void *malloc(size_t);
7373
void free(void *);
74+
void *memcpy(void *dest, const void *src, unsigned long n);
7475

7576
//===----------------------------------------------------------------------===
7677
// strlen()
@@ -1713,3 +1714,21 @@ void CWE124_Buffer_Underwrite__malloc_char_ncpy() {
17131714
free(dataBuffer);
17141715
}
17151716
#endif
1717+
1718+
#ifndef SUPPRESS_OUT_OF_BOUND
1719+
void CWE124_Buffer_Underwrite__malloc_char_memcpy() {
1720+
char * dataBuffer = (char *)malloc(100*sizeof(char));
1721+
if (dataBuffer == NULL) return;
1722+
memset(dataBuffer, 'A', 100-1);
1723+
dataBuffer[100-1] = '\0';
1724+
char *data = dataBuffer - 8;
1725+
1726+
char source[100];
1727+
memset(source, 'C', 100-1); // fill with 'C's
1728+
source[100-1] = '\0'; // null terminate
1729+
1730+
memcpy(data, source, 100*sizeof(char)); // expected-warning {{Memory copy function overflows the destination buffer}}
1731+
data[100-1] = '\0';
1732+
free(dataBuffer);
1733+
}
1734+
#endif

0 commit comments

Comments
 (0)