Skip to content

Commit c3a87dd

Browse files
committed
[analyzer] CStringChecker should check the first byte of the destination of strcpy, strncpy
By not checking if the first byte of the destination of strcpy and strncpy is writable, we missed some reports in the Juliet benchmark. (Juliet CWE-124 Buffer Underwrite: strcpy, strncpy) https://discourse.llvm.org/t/patches-inspired-by-the-juliet-benchmark/73106 Differential Revision: https://reviews.llvm.org/D159108
1 parent 4b9259b commit c3a87dd

File tree

2 files changed

+56
-0
lines changed

2 files changed

+56
-0
lines changed

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2009,6 +2009,11 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
20092009
SVal maxLastElement =
20102010
svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal, *maxLastNL, ptrTy);
20112011

2012+
// Check if the first byte of the destination is writable.
2013+
state = CheckLocation(C, state, Dst, DstVal, AccessKind::write);
2014+
if (!state)
2015+
return;
2016+
// Check if the last byte of the destination is writable.
20122017
state = CheckLocation(C, state, Dst, maxLastElement, AccessKind::write);
20132018
if (!state)
20142019
return;
@@ -2021,6 +2026,11 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
20212026

20222027
// ...and we haven't checked the bound, we'll check the actual copy.
20232028
if (!boundWarning) {
2029+
// Check if the first byte of the destination is writable.
2030+
state = CheckLocation(C, state, Dst, DstVal, AccessKind::write);
2031+
if (!state)
2032+
return;
2033+
// Check if the last byte of the destination is writable.
20242034
state = CheckLocation(C, state, Dst, lastElement, AccessKind::write);
20252035
if (!state)
20262036
return;

clang/test/Analysis/string.c

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1667,3 +1667,49 @@ void strcpy_no_overflow_2(char *y) {
16671667
strcpy(x, "12\0");
16681668
}
16691669
#endif
1670+
1671+
#ifndef SUPPRESS_OUT_OF_BOUND
1672+
void testStrcpyDestinationWritableFirstByte(void) {
1673+
char dst[10];
1674+
char *p = dst - 8;
1675+
strcpy(p, "src"); // expected-warning {{String copy function overflows the destination buffer}}
1676+
}
1677+
1678+
void CWE124_Buffer_Underwrite__malloc_char_cpy() {
1679+
char * dataBuffer = (char *)malloc(100*sizeof(char));
1680+
if (dataBuffer == NULL) return;
1681+
memset(dataBuffer, 'A', 100-1);
1682+
dataBuffer[100-1] = '\0';
1683+
char * data = dataBuffer - 8;
1684+
char source[100];
1685+
memset(source, 'C', 100-1); // fill with 'C's
1686+
source[100-1] = '\0'; // null terminate
1687+
strcpy(data, source); // expected-warning {{String copy function overflows the destination buffer}}
1688+
free(dataBuffer);
1689+
}
1690+
#endif
1691+
1692+
#ifndef SUPPRESS_OUT_OF_BOUND
1693+
void testStrncpyDestinationWritableFirstByte(void) {
1694+
char source[100];
1695+
use_string(source); // escape
1696+
char buf[100];
1697+
char *p = buf - 8;
1698+
strncpy(p, source, 100-1); // expected-warning {{String copy function overflows the destination buffer}}
1699+
}
1700+
1701+
void CWE124_Buffer_Underwrite__malloc_char_ncpy() {
1702+
char * dataBuffer = (char *)malloc(100*sizeof(char));
1703+
if (dataBuffer == 0) return;
1704+
memset(dataBuffer, 'A', 100-1);
1705+
dataBuffer[100-1] = '\0';
1706+
char *data = dataBuffer - 8;
1707+
1708+
char source[100];
1709+
memset(source, 'C', 100-1); // fill with 'C's
1710+
source[100-1] = '\0'; // null terminate
1711+
strncpy(data, source, 100-1); // expected-warning {{String copy function overflows the destination buffer}}
1712+
data[100-1] = '\0'; // null terminate
1713+
free(dataBuffer);
1714+
}
1715+
#endif

0 commit comments

Comments
 (0)