From: George Karpenkov Date: Thu, 23 Aug 2018 00:02:35 +0000 (+0000) Subject: Revert "[CStringSyntaxChecker] Check strlcat sizeof check" X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0a2e96aedf66313c4f1ad375917965c915dabb1d;p=clang Revert "[CStringSyntaxChecker] Check strlcat sizeof check" This reverts commit 3073790e87378fea9a68fb052185fec9596ef135. The check is not correct, strlact(dest, "mystr", sizeof(dest)) is fine. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@340501 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp index 2763408fac..8b4aa857e7 100644 --- a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp @@ -90,16 +90,7 @@ class WalkAST: public StmtVisitor { /// strlcpy(dst, "abcd", 4); /// strlcpy(dst + 3, "abcd", 2); /// strlcpy(dst, "abcd", cpy); - /// Identify erroneous patterns in the last argument to strlcat - the number - /// of bytes to copy. - /// The bad pattern checked is when the last argument is basically - /// pointing to the destination buffer size or argument larger or - /// equal to. - /// char dst[2]; - /// strlcat(dst, src2, sizeof(dst)); - /// strlcat(dst, src2, 2); - /// strlcat(dst, src2, 10); - bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE); + bool containsBadStrlcpyPattern(const CallExpr *CE); public: WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC) @@ -151,21 +142,15 @@ bool WalkAST::containsBadStrncatPattern(const CallExpr *CE) { return false; } -bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) { +bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) { if (CE->getNumArgs() != 3) return false; - const FunctionDecl *FD = CE->getDirectCallee(); - bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat"); const Expr *DstArg = CE->getArg(0); const Expr *LenArg = CE->getArg(2); const auto *DstArgDecl = dyn_cast(DstArg->IgnoreParenImpCasts()); const auto *LenArgDecl = dyn_cast(LenArg->IgnoreParenLValueCasts()); uint64_t DstOff = 0; - // - sizeof(dst) - // strlcat appends at most size - strlen(dst) - 1 - if (Append && isSizeof(LenArg, DstArg)) - return true; // - size_t dstlen = sizeof(dst) if (LenArgDecl) { const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl()); @@ -196,10 +181,7 @@ bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) { if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) { ASTContext &C = BR.getContext(); uint64_t BufferLen = C.getTypeSize(Buffer) / 8; - auto RemainingBufferLen = BufferLen - DstOff; - if (Append) - RemainingBufferLen -= 1; - if (RemainingBufferLen < ILRawVal) + if ((BufferLen - DstOff) < ILRawVal) return true; } } @@ -238,7 +220,7 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { LenArg->getSourceRange()); } } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) { - if (containsBadStrlcpyStrlcatPattern(CE)) { + if (containsBadStrlcpyPattern(CE)) { const Expr *DstArg = CE->getArg(0); const Expr *LenArg = CE->getArg(2); PathDiagnosticLocation Loc = @@ -252,34 +234,6 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { if (!DstName.empty()) os << "Replace with the value 'sizeof(" << DstName << ")` or lower"; - BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument", - "C String API", os.str(), Loc, - LenArg->getSourceRange()); - } - } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) { - if (containsBadStrlcpyStrlcatPattern(CE)) { - const Expr *DstArg = CE->getArg(0); - const Expr *LenArg = CE->getArg(2); - PathDiagnosticLocation Loc = - PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), AC); - - StringRef DstName = getPrintableName(DstArg); - StringRef LenName = getPrintableName(LenArg); - - SmallString<256> S; - llvm::raw_svector_ostream os(S); - os << "The third argument allows to potentially copy more bytes than it should. "; - os << "Replace with the value "; - if (!LenName.empty()) - os << "'" << LenName << "'"; - else - os << " "; - if (!DstName.empty()) - os << " - strlen(" << DstName << ")"; - else - os << " - strlen()"; - os << " - 1 or lower"; - BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument", "C String API", os.str(), Loc, LenArg->getSourceRange()); diff --git a/test/Analysis/cstring-syntax.c b/test/Analysis/cstring-syntax.c index c6d3694060..fe1253bedb 100644 --- a/test/Analysis/cstring-syntax.c +++ b/test/Analysis/cstring-syntax.c @@ -7,7 +7,6 @@ typedef __SIZE_TYPE__ size_t; char *strncat(char *, const char *, size_t); size_t strlen (const char *s); size_t strlcpy(char *, const char *, size_t); -size_t strlcat(char *, const char *, size_t); void testStrncat(const char *src) { char dest[10]; @@ -34,19 +33,3 @@ void testStrlcpy(const char *src) { strlcpy(dest + 5, src, 5); strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}} } - -void testStrlcat(const char *src) { - char dest[10]; - size_t badlen = 10; - size_t ulen; - strlcpy(dest, "aaaaa", sizeof("aaaaa") - 1); - strlcat(dest, "bbbb", (sizeof("bbbb") - 1) - sizeof(dest) - 1); - strlcpy(dest, "012345678", sizeof(dest)); - strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value - strlen(dest) - 1 or lower}} - strlcpy(dest, "0123456789", sizeof(dest)); - strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(dest) - 1 or lower}} - strlcat(dest, "0123456789", badlen - strlen(dest) - 1); - strlcat(dest, src, ulen); - 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 'badlen' - strlen() - 1 or lower}} -}