[cfe-commits] r133472 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/CStringChecker.cpp test/Analysis/string.c
Jordy Rose
jediknil at belkadan.com
Mon Jun 20 14:55:40 PDT 2011
Author: jrose
Date: Mon Jun 20 16:55:40 2011
New Revision: 133472
URL: http://llvm.org/viewvc/llvm-project?rev=133472&view=rev
Log:
[analyzer] Finish size argument checking for strncat (and strncpy).
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
cfe/trunk/test/Analysis/string.c
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp?rev=133472&r1=133471&r2=133472&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp Mon Jun 20 16:55:40 2011
@@ -1217,8 +1217,14 @@
SValBuilder &svalBuilder = C.getSValBuilder();
QualType cmpTy = svalBuilder.getConditionType();
+ QualType sizeTy = svalBuilder.getContext().getSizeType();
+ // These two values allow checking two kinds of errors:
+ // - actual overflows caused by a source that doesn't fit in the destination
+ // - potential overflows caused by a bound that could exceed the destination
SVal amountCopied = UnknownVal();
+ SVal maxLastElementIndex = UnknownVal();
+ const char *boundWarning = NULL;
// If the function is strncpy, strncat, etc... it is bounded.
if (isBounded) {
@@ -1227,9 +1233,7 @@
SVal lenVal = state->getSVal(lenExpr);
// Protect against misdeclared strncpy().
- lenVal = svalBuilder.evalCast(lenVal,
- svalBuilder.getContext().getSizeType(),
- lenExpr->getType());
+ lenVal = svalBuilder.evalCast(lenVal, sizeTy, lenExpr->getType());
NonLoc *strLengthNL = dyn_cast<NonLoc>(&strLength);
NonLoc *lenValNL = dyn_cast<NonLoc>(&lenVal);
@@ -1240,9 +1244,11 @@
const GRState *stateSourceTooLong, *stateSourceNotTooLong;
// Check if the max number to copy is less than the length of the src.
+ // If the bound is equal to the source length, strncpy won't null-
+ // terminate the result!
llvm::tie(stateSourceTooLong, stateSourceNotTooLong) =
state->assume(cast<DefinedOrUnknownSVal>
- (svalBuilder.evalBinOpNN(state, BO_GT, *strLengthNL,
+ (svalBuilder.evalBinOpNN(state, BO_GE, *strLengthNL,
*lenValNL, cmpTy)));
if (stateSourceTooLong && !stateSourceNotTooLong) {
@@ -1259,19 +1265,43 @@
}
// We still want to know if the bound is known to be too large.
- const char * const warningMsg =
- "Size argument is greater than the length of the destination buffer";
- state = CheckBufferAccess(C, state, lenExpr, Dst, warningMsg,
- /* WarnAboutSize = */ true);
- // FIXME: It'd be nice for this not to be a hard error, so we can warn
- // about more than one thing, but the multiple calls to CheckLocation
- // result in the same ExplodedNode, which means we don't keep emitting
- // bug reports.
- if (!state)
- return;
+ if (lenValNL) {
+ if (isAppending) {
+ // For strncat, the check is strlen(dst) + lenVal < sizeof(dst)
+
+ // Get the string length of the destination. If the destination is
+ // memory that can't have a string length, we shouldn't be copying
+ // into it anyway.
+ SVal dstStrLength = getCStringLength(C, state, Dst, DstVal);
+ if (dstStrLength.isUndef())
+ return;
+
+ if (NonLoc *dstStrLengthNL = dyn_cast<NonLoc>(&dstStrLength)) {
+ maxLastElementIndex = svalBuilder.evalBinOpNN(state, BO_Add,
+ *lenValNL,
+ *dstStrLengthNL,
+ sizeTy);
+ boundWarning = "Size argument is greater than the free space in the "
+ "destination buffer";
+ }
+
+ } else {
+ // For strncpy, this is just checking that lenVal <= sizeof(dst)
+ // (Yes, strncpy and strncat differ in how they treat termination.
+ // strncat ALWAYS terminates, but strncpy doesn't.)
+ NonLoc one = cast<NonLoc>(svalBuilder.makeIntVal(1, sizeTy));
+ maxLastElementIndex = svalBuilder.evalBinOpNN(state, BO_Sub, *lenValNL,
+ one, sizeTy);
+ boundWarning = "Size argument is greater than the length of the "
+ "destination buffer";
+ }
+ }
// If we couldn't pin down the copy length, at least bound it.
- if (amountCopied.isUnknown()) {
+ // FIXME: We should actually run this code path for append as well, but
+ // right now it creates problems with constraints (since we can end up
+ // trying to pass constraints from symbol to symbol).
+ if (amountCopied.isUnknown() && !isAppending) {
// Try to get a "hypothetical" string length symbol, which we can later
// set as a real value if that turns out to be the case.
amountCopied = getCStringLength(C, state, lenExpr, srcVal, true);
@@ -1327,8 +1357,6 @@
if (dstStrLength.isUndef())
return;
- QualType sizeTy = svalBuilder.getContext().getSizeType();
-
NonLoc *srcStrLengthNL = dyn_cast<NonLoc>(&amountCopied);
NonLoc *dstStrLengthNL = dyn_cast<NonLoc>(&dstStrLength);
@@ -1393,17 +1421,34 @@
// If the destination is a MemRegion, try to check for a buffer overflow and
// record the new string length.
if (loc::MemRegionVal *dstRegVal = dyn_cast<loc::MemRegionVal>(&DstVal)) {
- // If the final length is known, we can check for an overflow.
+ QualType ptrTy = Dst->getType();
+
+ // If we have an exact value on a bounded copy, use that to check for
+ // overflows, rather than our estimate about how much is actually copied.
+ if (boundWarning) {
+ if (NonLoc *maxLastNL = dyn_cast<NonLoc>(&maxLastElementIndex)) {
+ SVal maxLastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal,
+ *maxLastNL, ptrTy);
+ state = CheckLocation(C, state, CE->getArg(2), maxLastElement,
+ boundWarning);
+ if (!state)
+ return;
+ }
+ }
+
+ // Then, if the final length is known...
if (NonLoc *knownStrLength = dyn_cast<NonLoc>(&finalStrLength)) {
SVal lastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal,
- *knownStrLength,
- Dst->getType());
+ *knownStrLength, ptrTy);
- const char * const warningMsg =
- "String copy function overflows destination buffer";
- state = CheckLocation(C, state, Dst, lastElement, warningMsg);
- if (!state)
- return;
+ // ...and we haven't checked the bound, we'll check the actual copy.
+ if (!boundWarning) {
+ const char * const warningMsg =
+ "String copy function overflows destination buffer";
+ state = CheckLocation(C, state, Dst, lastElement, warningMsg);
+ if (!state)
+ return;
+ }
// If this is a stpcpy-style copy, the last element is the return value.
if (returnEnd)
@@ -1419,10 +1464,15 @@
state = InvalidateBuffer(C, state, Dst, *dstRegVal);
// Set the C string length of the destination, if we know it.
- if (!isBounded || (amountCopied == strLength))
- state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength);
- else
- state = setCStringLength(state, dstRegVal->getRegion(), UnknownVal());
+ if (isBounded && !isAppending) {
+ // strncpy is annoying in that it doesn't guarantee to null-terminate
+ // the result string. If the original string didn't fit entirely inside
+ // the bound (including the null-terminator), we don't know how long the
+ // result is.
+ if (amountCopied != strLength)
+ finalStrLength = UnknownVal();
+ }
+ state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength);
}
assert(state);
Modified: cfe/trunk/test/Analysis/string.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/string.c?rev=133472&r1=133471&r2=133472&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/string.c (original)
+++ cfe/trunk/test/Analysis/string.c Mon Jun 20 16:55:40 2011
@@ -604,25 +604,25 @@
void strncat_overflow_0(char *y) {
char x[4] = "12";
if (strlen(y) == 4)
- strncat(x, y, strlen(y)); // expected-warning{{String copy function overflows destination buffer}}
+ strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
}
void strncat_overflow_1(char *y) {
char x[4] = "12";
if (strlen(y) == 3)
- strncat(x, y, strlen(y)); // expected-warning{{String copy function overflows destination buffer}}
+ strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
}
void strncat_overflow_2(char *y) {
char x[4] = "12";
if (strlen(y) == 2)
- strncat(x, y, strlen(y)); // expected-warning{{String copy function overflows destination buffer}}
+ strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
}
void strncat_overflow_3(char *y) {
char x[4] = "12";
if (strlen(y) == 4)
- strncat(x, y, 2); // expected-warning{{String copy function overflows destination buffer}}
+ strncat(x, y, 2); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
}
void strncat_no_overflow_1(char *y) {
char x[5] = "12";
@@ -636,6 +636,63 @@
strncat(x, y, 1); // no-warning
}
+void strncat_symbolic_dst_length(char *dst) {
+ strncat(dst, "1234", 5);
+ if (strlen(dst) < 4)
+ (void)*(char*)0; // no-warning
+}
+
+void strncat_symbolic_src_length(char *src) {
+ char dst[8] = "1234";
+ strncat(dst, src, 3);
+ if (strlen(dst) < 4)
+ (void)*(char*)0; // no-warning
+
+ char dst2[8] = "1234";
+ strncat(dst2, src, 4); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
+}
+
+void strncat_unknown_src_length(char *src, int offset) {
+ char dst[8] = "1234";
+ strncat(dst, &src[offset], 3);
+ if (strlen(dst) < 4)
+ (void)*(char*)0; // no-warning
+
+ char dst2[8] = "1234";
+ strncat(dst2, &src[offset], 4); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
+}
+
+// There is no strncat_unknown_dst_length because if we can't get a symbolic
+// length for the "before" strlen, we won't be able to set one for "after".
+
+void strncat_symbolic_limit(unsigned limit) {
+ char dst[6] = "1234";
+ char src[] = "567";
+ strncat(dst, src, limit); // no-warning
+ if (strlen(dst) < 4)
+ (void)*(char*)0; // no-warning
+ if (strlen(dst) == 4)
+ (void)*(char*)0; // expected-warning{{null}}
+}
+
+void strncat_unknown_limit(float limit) {
+ char dst[6] = "1234";
+ char src[] = "567";
+ strncat(dst, src, (size_t)limit); // no-warning
+ if (strlen(dst) < 4)
+ (void)*(char*)0; // no-warning
+ if (strlen(dst) == 4)
+ (void)*(char*)0; // expected-warning{{null}}
+}
+
+void strncat_too_big(char *dst, char *src) {
+ if (strlen(dst) != (((size_t)0) - 2))
+ return;
+ if (strlen(src) != 2)
+ return;
+ strncat(dst, src, 2); // expected-warning{{This expression will create a string whose length is too big to be represented as a size_t}}
+}
+
//===----------------------------------------------------------------------===
// strcmp()
//===----------------------------------------------------------------------===
More information about the cfe-commits
mailing list