Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -28,6 +28,7 @@ using namespace ento; namespace { +enum class appendKind { none = 0, strcat = 1, strlcat = 2 }; class CStringChecker : public Checker< eval::Call, check::PreStmt, check::LiveSymbols, @@ -129,11 +130,8 @@ void evalStrncpy(CheckerContext &C, const CallExpr *CE) const; void evalStpcpy(CheckerContext &C, const CallExpr *CE) const; void evalStrlcpy(CheckerContext &C, const CallExpr *CE) const; - void evalStrcpyCommon(CheckerContext &C, - const CallExpr *CE, - bool returnEnd, - bool isBounded, - bool isAppending, + void evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool returnEnd, + bool isBounded, appendKind appendK, bool returnPtr = true) const; void evalStrcat(CheckerContext &C, const CallExpr *CE) const; @@ -1473,7 +1471,7 @@ evalStrcpyCommon(C, CE, /* returnEnd = */ false, /* isBounded = */ false, - /* isAppending = */ false); + /* appendK = */ appendKind::none); } void CStringChecker::evalStrncpy(CheckerContext &C, const CallExpr *CE) const { @@ -1481,7 +1479,7 @@ evalStrcpyCommon(C, CE, /* returnEnd = */ false, /* isBounded = */ true, - /* isAppending = */ false); + /* appendK = */ appendKind::none); } void CStringChecker::evalStpcpy(CheckerContext &C, const CallExpr *CE) const { @@ -1489,24 +1487,24 @@ evalStrcpyCommon(C, CE, /* returnEnd = */ true, /* isBounded = */ false, - /* isAppending = */ false); + /* appendK = */ appendKind::none); } void CStringChecker::evalStrlcpy(CheckerContext &C, const CallExpr *CE) const { - // char *strlcpy(char *dst, const char *src, size_t n); + // size_t strlcpy(char *dest, const char *src, size_t size); evalStrcpyCommon(C, CE, /* returnEnd = */ true, /* isBounded = */ true, - /* isAppending = */ false, + /* appendK = */ appendKind::none, /* returnPtr = */ false); } void CStringChecker::evalStrcat(CheckerContext &C, const CallExpr *CE) const { - //char *strcat(char *restrict s1, const char *restrict s2); + // char *strcat(char *restrict s1, const char *restrict s2); evalStrcpyCommon(C, CE, /* returnEnd = */ false, /* isBounded = */ false, - /* isAppending = */ true); + /* appendK = */ appendKind::strcat); } void CStringChecker::evalStrncat(CheckerContext &C, const CallExpr *CE) const { @@ -1514,25 +1512,24 @@ evalStrcpyCommon(C, CE, /* returnEnd = */ false, /* isBounded = */ true, - /* isAppending = */ true); + /* appendK = */ appendKind::strcat); } void CStringChecker::evalStrlcat(CheckerContext &C, const CallExpr *CE) const { - // FIXME: strlcat() uses a different rule for bound checking, i.e. 'n' means - // a different thing as compared to strncat(). This currently causes - // false positives in the alpha string bound checker. - - //char *strlcat(char *s1, const char *s2, size_t n); + // size_t strlcat(char *dst, const char *src, size_t size); + // It will append at most size - strlen(dst) - 1 bytes, + // NULL-terminating the result. evalStrcpyCommon(C, CE, /* returnEnd = */ false, /* isBounded = */ true, - /* isAppending = */ true, + /* appendK = */ appendKind::strlcat, /* returnPtr = */ false); } void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool returnEnd, bool isBounded, - bool isAppending, bool returnPtr) const { + appendKind appendK, + bool returnPtr) const { CurrentFunctionDescription = "string copy function"; ProgramStateRef state = C.getState(); const LocationContext *LCtx = C.getLocationContext(); @@ -1554,6 +1551,11 @@ // Get the string length of the source. SVal strLength = getCStringLength(C, state, srcExpr, srcVal); + Optional strLengthNL = strLength.getAs(); + + // Get the string length of the destination + SVal dstStrLength = getCStringLength(C, state, Dst, DstVal); + Optional dstStrLengthNL = dstStrLength.getAs(); // If the source isn't a valid C string, give up. if (strLength.isUndef()) @@ -1584,7 +1586,6 @@ // Protect against misdeclared strncpy(). lenVal = svalBuilder.evalCast(lenVal, sizeTy, lenExpr->getType()); - Optional strLengthNL = strLength.getAs(); Optional lenValNL = lenVal.getAs(); // If we know both values, we might be able to figure out how much @@ -1592,12 +1593,15 @@ if (strLengthNL && lenValNL) { ProgramStateRef 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! - std::tie(stateSourceTooLong, stateSourceNotTooLong) = state->assume( - svalBuilder.evalBinOpNN(state, BO_GE, *strLengthNL, *lenValNL, cmpTy) - .castAs()); + if (appendK == appendKind::none || appendK == appendKind::strcat) { + ProgramStateRef 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! + std::tie(stateSourceTooLong, stateSourceNotTooLong) = state->assume( + svalBuilder + .evalBinOpNN(state, BO_GE, *strLengthNL, *lenValNL, cmpTy) + .castAs()); if (stateSourceTooLong && !stateSourceNotTooLong) { // Max number to copy is less than the length of the src, so the actual @@ -1605,36 +1609,69 @@ state = stateSourceTooLong; amountCopied = lenVal; - } else if (!stateSourceTooLong && stateSourceNotTooLong) { - // The source buffer entirely fits in the bound. - state = stateSourceNotTooLong; - amountCopied = strLength; + } else if (!stateSourceTooLong && stateSourceNotTooLong) { + // The source buffer entirely fits in the bound. + state = stateSourceNotTooLong; + amountCopied = strLength; + } + } else if (appendK == appendKind::strlcat && dstStrLengthNL) { + // amountCopied = min (size - dstLen - 1 , srcLen) + + SVal freeSpace = svalBuilder.evalBinOpNN(state, BO_Sub, *lenValNL, + *dstStrLengthNL, sizeTy); + + if (freeSpace.getAs()) { + freeSpace = + svalBuilder.evalBinOp(state, BO_Sub, freeSpace, + svalBuilder.makeIntVal(1, sizeTy), sizeTy); + Optional freeSpaceNL = freeSpace.getAs(); + + SVal haveEnoughSpace = svalBuilder.evalBinOpNN( + state, BO_LE, *strLengthNL, *freeSpaceNL, cmpTy); + + ProgramStateRef TrueState, FalseState; + std::tie(TrueState, FalseState) = + state->assume(haveEnoughSpace.castAs()); + + if (TrueState && !FalseState) { + // srcStrLength <= size - dstStrLength -1 + amountCopied = strLength; + } + + if (!TrueState && FalseState) { + // srcStrLength > size - dstStrLength -1 + amountCopied = freeSpace; + } + + if (TrueState && FalseState) { + amountCopied = UnknownVal(); + } + } } } // We still want to know if the bound is known to be too large. if (lenValNL) { - if (isAppending) { + if (appendK == appendKind::strcat) { // 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 (Optional dstStrLengthNL = dstStrLength.getAs()) { - maxLastElementIndex = svalBuilder.evalBinOpNN(state, BO_Add, - *lenValNL, - *dstStrLengthNL, - sizeTy); + if (dstStrLengthNL) { + 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) + // For strncpy and strlcat, 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.) @@ -1650,7 +1687,16 @@ if (returnPtr) { StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, DstVal); } else { - StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, *lenValNL); + if (appendK == appendKind::none) { + // strlcpy returns strlen(src) + StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, *strLengthNL); + } else if (dstStrLengthNL) { + // strlcat returns strlen(src) + strlen(dst) + SVal retSize = svalBuilder.evalBinOpNN( + state, BO_Add, *strLengthNL, *dstStrLengthNL, sizeTy); + StateZeroSize = + StateZeroSize->BindExpr(CE, LCtx, *(retSize.getAs())); + } } C.addTransition(StateZeroSize); return; @@ -1666,44 +1712,6 @@ "destination buffer"; } } - - // If we couldn't pin down the copy length, at least bound it. - // 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); - assert(!amountCopied.isUndef()); - - if (Optional amountCopiedNL = amountCopied.getAs()) { - if (lenValNL) { - // amountCopied <= lenVal - SVal copiedLessThanBound = svalBuilder.evalBinOpNN(state, BO_LE, - *amountCopiedNL, - *lenValNL, - cmpTy); - state = state->assume( - copiedLessThanBound.castAs(), true); - if (!state) - return; - } - - if (strLengthNL) { - // amountCopied <= strlen(source) - SVal copiedLessThanSrc = svalBuilder.evalBinOpNN(state, BO_LE, - *amountCopiedNL, - *strLengthNL, - cmpTy); - state = state->assume( - copiedLessThanSrc.castAs(), true); - if (!state) - return; - } - } - } - } else { // The function isn't bounded. The amount copied should match the length // of the source buffer. @@ -1716,28 +1724,37 @@ // buffer. (It may not actually be the strlen if the destination buffer // is not terminated.) SVal finalStrLength = UnknownVal(); + SVal strlRetVal = UnknownVal(); + + if (appendK == appendKind::none && !returnPtr) { + // strlcpy returns the sizeof(src) + strlRetVal = strLength; + } // If this is an appending function (strcat, strncat...) then set the // string length to strlen(src) + strlen(dst) since the buffer will // ultimately contain both. - if (isAppending) { + if (appendK != appendKind::none) { // 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; - Optional srcStrLengthNL = amountCopied.getAs(); - Optional dstStrLengthNL = dstStrLength.getAs(); + if (appendK == appendKind::strlcat && dstStrLengthNL && strLengthNL) { + strlRetVal = svalBuilder.evalBinOpNN(state, BO_Add, *strLengthNL, + *dstStrLengthNL, sizeTy); + } + + Optional amountCopiedNL = amountCopied.getAs(); // If we know both string lengths, we might know the final string length. - if (srcStrLengthNL && dstStrLengthNL) { + if (amountCopiedNL && dstStrLengthNL) { // Make sure the two lengths together don't overflow a size_t. - state = checkAdditionOverflow(C, state, *srcStrLengthNL, *dstStrLengthNL); + state = checkAdditionOverflow(C, state, *amountCopiedNL, *dstStrLengthNL); if (!state) return; - finalStrLength = svalBuilder.evalBinOpNN(state, BO_Add, *srcStrLengthNL, + finalStrLength = svalBuilder.evalBinOpNN(state, BO_Add, *amountCopiedNL, *dstStrLengthNL, sizeTy); } @@ -1750,19 +1767,19 @@ assert(!finalStrLength.isUndef()); if (Optional finalStrLengthNL = finalStrLength.getAs()) { - if (srcStrLengthNL) { + if (amountCopiedNL && appendK == appendKind::none) { + // we overwrite dst string with the src // finalStrLength >= srcStrLength - SVal sourceInResult = svalBuilder.evalBinOpNN(state, BO_GE, - *finalStrLengthNL, - *srcStrLengthNL, - cmpTy); + SVal sourceInResult = svalBuilder.evalBinOpNN( + state, BO_GE, *finalStrLengthNL, *amountCopiedNL, cmpTy); state = state->assume(sourceInResult.castAs(), true); if (!state) return; } - if (dstStrLengthNL) { + if (dstStrLengthNL && appendK != appendKind::none) { + // we extend the dst string with the src // finalStrLength >= dstStrLength SVal destInResult = svalBuilder.evalBinOpNN(state, BO_GE, *finalStrLengthNL, @@ -1789,7 +1806,11 @@ // copied element, or a pointer to the start of the destination buffer. Result = (returnEnd ? UnknownVal() : DstVal); } else { - Result = finalStrLength; + if (appendK == appendKind::strlcat || appendK == appendKind::none) + //strlcpy, strlcat + Result = strlRetVal; + else + Result = finalStrLength; } assert(state); @@ -1848,7 +1869,7 @@ nullptr); // Set the C string length of the destination, if we know it. - if (isBounded && !isAppending) { + if (isBounded && (appendK == appendKind::none)) { // 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 Index: test/Analysis/bsd-string.c =================================================================== --- test/Analysis/bsd-string.c +++ test/Analysis/bsd-string.c @@ -9,6 +9,7 @@ typedef __typeof(sizeof(int)) size_t; size_t strlcpy(char *dst, const char *src, size_t n); size_t strlcat(char *dst, const char *src, size_t n); +size_t strlen(const char *s); void clang_analyzer_eval(int); void f1() { @@ -18,9 +19,11 @@ void f2() { char buf[5]; - strlcpy(buf, "abcd", sizeof(buf)); // expected-no-warning - // FIXME: This should not warn. The string is safely truncated. - strlcat(buf, "efgh", sizeof(buf)); // expected-warning{{Size argument is greater than the free space in the destination buffer}} + size_t len; + len = strlcpy(buf, "abcd", sizeof(buf)); // expected-no-warning + clang_analyzer_eval(len == 4); // expected-warning{{TRUE}} + len = strlcat(buf, "efgh", sizeof(buf)); // expected-no-warning + clang_analyzer_eval(len == 8); // expected-warning{{TRUE}} } void f3() { @@ -44,7 +47,80 @@ clang_analyzer_eval(len == 7); // expected-warning{{TRUE}} } + int f7() { char buf[8]; return strlcpy(buf, "1234567", 0); // no-crash } + +void f8(){ + char buf[5]; + size_t len; + + // basic strlcpy + len = strlcpy(buf,"123", sizeof(buf)); + clang_analyzer_eval(len==3);// expected-warning{{TRUE}} + len = strlen(buf); + clang_analyzer_eval(len==3);// expected-warning{{TRUE}} + + // testing bounded strlcat + len = strlcat(buf,"456", sizeof(buf)); + clang_analyzer_eval(len==6);// expected-warning{{TRUE}} + len = strlen(buf); + clang_analyzer_eval(len==4);// expected-warning{{TRUE}} + + // testing strlcat with size==0 + len = strlcat(buf,"789", 0); + clang_analyzer_eval(len==7);// expected-warning{{TRUE}} + len = strlen(buf); + clang_analyzer_eval(len==4);// expected-warning{{TRUE}} + + // testing strlcpy with size==0 + len = strlcpy(buf,"123",0); + clang_analyzer_eval(len==3);// expected-warning{{TRUE}} + len = strlen(buf); + clang_analyzer_eval(len==4);// expected-warning{{TRUE}} + +} + +void f9(int unknown_size, char* unknown_src, char* unknown_dst){ + char buf[8]; + size_t len; + + len = strlcpy(buf,"abba",sizeof(buf)); + + clang_analyzer_eval(len==4);// expected-warning{{TRUE}} + clang_analyzer_eval(strlen(buf)==4);// expected-warning{{TRUE}} + + //size is unknown + len = strlcat(buf,"cd", unknown_size); + clang_analyzer_eval(len==6);// expected-warning{{TRUE}} + clang_analyzer_eval(strlen(buf)>=4);// expected-warning{{TRUE}} + + //dst is unknown + len = strlcpy(unknown_dst,"abbc",unknown_size); + clang_analyzer_eval(len==4);// expected-warning{{TRUE}} + clang_analyzer_eval(strlen(unknown_dst));// expected-warning{{UNKNOWN}} + + //src is unknown + len = strlcpy(buf,unknown_src, sizeof(buf)); + clang_analyzer_eval(len);// expected-warning{{UNKNOWN}} + clang_analyzer_eval(strlen(buf));// expected-warning{{UNKNOWN}} + + //src, dst is unknown + len = strlcpy(unknown_dst, unknown_src, unknown_size); + clang_analyzer_eval(len);// expected-warning{{UNKNOWN}} + clang_analyzer_eval(strlen(unknown_dst));// expected-warning{{UNKNOWN}} + + //size is unknown + len = strlcat(buf+2,unknown_src+1, sizeof(buf));// expected-warning{{Size argument is greater than the length of the destination buffer}}; +} + +void f10(){ + char buf[8]; + size_t len; + + len = strlcpy(buf,"abba",sizeof(buf)); + clang_analyzer_eval(len==4);// expected-warning{{TRUE}} + strlcat(buf, "efghi",9);// expected-warning{{Size argument is greater than the length of the destination buffer}} +} Index: test/Analysis/cstring-syntax.c =================================================================== --- test/Analysis/cstring-syntax.c +++ test/Analysis/cstring-syntax.c @@ -53,4 +53,10 @@ strlcpy(dest, src, 5); strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() or lower}} strlcat(dest, "aaaaaaaaaaaaaaa", 10); // no-warning + + //test for Bug 41729 + char a[256], b[256]; + strlcpy(a, "world", sizeof(a)); + strlcpy(b, "hello ", sizeof(b)); + strlcat(b, a, sizeof(b)); // no-warning }