From: David Carlier Date: Tue, 14 Aug 2018 05:12:53 +0000 (+0000) Subject: [CStringSyntaxChecker] Check strlcat sizeof check X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=731b1c80ca380ee4d0e68704f8185bb523ce7b23;p=clang [CStringSyntaxChecker] Check strlcat sizeof check - Assuming strlcat is used with strlcpy we check as we can if the last argument does not equal os not larger than the buffer. - Advising the proper usual pattern. Reviewers: NoQ, george.karpenkov Reviewed By: george.karpenkov Differential Revision: https://reviews.llvm.org/D49722 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@339641 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp index 8b4aa857e7..2763408fac 100644 --- a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp @@ -90,7 +90,16 @@ class WalkAST: public StmtVisitor { /// strlcpy(dst, "abcd", 4); /// strlcpy(dst + 3, "abcd", 2); /// strlcpy(dst, "abcd", cpy); - bool containsBadStrlcpyPattern(const CallExpr *CE); + /// 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); public: WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC) @@ -142,15 +151,21 @@ bool WalkAST::containsBadStrncatPattern(const CallExpr *CE) { return false; } -bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) { +bool WalkAST::containsBadStrlcpyStrlcatPattern(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()); @@ -181,7 +196,10 @@ bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) { if (const auto *Buffer = dyn_cast(DstArgDecl->getType())) { ASTContext &C = BR.getContext(); uint64_t BufferLen = C.getTypeSize(Buffer) / 8; - if ((BufferLen - DstOff) < ILRawVal) + auto RemainingBufferLen = BufferLen - DstOff; + if (Append) + RemainingBufferLen -= 1; + if (RemainingBufferLen < ILRawVal) return true; } } @@ -220,7 +238,7 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { LenArg->getSourceRange()); } } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) { - if (containsBadStrlcpyPattern(CE)) { + if (containsBadStrlcpyStrlcatPattern(CE)) { const Expr *DstArg = CE->getArg(0); const Expr *LenArg = CE->getArg(2); PathDiagnosticLocation Loc = @@ -234,6 +252,34 @@ 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 fe1253bedb..c6d3694060 100644 --- a/test/Analysis/cstring-syntax.c +++ b/test/Analysis/cstring-syntax.c @@ -7,6 +7,7 @@ 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]; @@ -33,3 +34,19 @@ 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}} +}