From: David Carlier Date: Sun, 23 Sep 2018 08:30:17 +0000 (+0000) Subject: [CStringSyntaxChecker] Check strlcat sizeof check X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=62d19fada46aae067969bd9c36538c13187ca247;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: george.karpenkov, NoQ, MaskRay Reviewed By: MaskRay Differential Revision: https://reviews.llvm.org/D49722 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@342832 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp index 8b4aa857e7..b486443ac0 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,19 @@ 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; + if (isSizeof(LenArg, DstArg)) + return false; // - size_t dstlen = sizeof(dst) if (LenArgDecl) { const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl()); @@ -181,8 +194,14 @@ 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) - return true; + auto RemainingBufferLen = BufferLen - DstOff; + if (Append) { + if (RemainingBufferLen <= ILRawVal) + return true; + } else { + if (RemainingBufferLen < ILRawVal) + return true; + } } } } @@ -219,8 +238,9 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { "C String API", os.str(), Loc, LenArg->getSourceRange()); } - } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) { - if (containsBadStrlcpyPattern(CE)) { + } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy") || + CheckerContext::isCLibraryFunction(FD, "strlcat")) { + if (containsBadStrlcpyStrlcatPattern(CE)) { const Expr *DstArg = CE->getArg(0); const Expr *LenArg = CE->getArg(2); PathDiagnosticLocation Loc = @@ -230,13 +250,17 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { SmallString<256> S; llvm::raw_svector_ostream os(S); - os << "The third argument is larger than the size of the input buffer. "; + os << "The third argument allows to potentially copy more bytes than it should. "; + os << "Replace with the value "; if (!DstName.empty()) - os << "Replace with the value 'sizeof(" << DstName << ")` or lower"; + os << "sizeof(" << DstName << ")"; + else + os << "sizeof()"; + os << " or lower"; BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument", - "C String API", os.str(), Loc, - LenArg->getSourceRange()); + "C String API", os.str(), Loc, + LenArg->getSourceRange()); } } diff --git a/test/Analysis/cstring-syntax.c b/test/Analysis/cstring-syntax.c index fe1253bedb..d2e12e8303 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]; @@ -27,9 +28,27 @@ void testStrlcpy(const char *src) { strlcpy(dest, src, sizeof(dest)); strlcpy(dest, src, destlen); strlcpy(dest, src, 10); - strlcpy(dest, src, 20); // expected-warning {{The third argument is larger than the size of the input buffer. Replace with the value 'sizeof(dest)` or lower}} - strlcpy(dest, src, badlen); // expected-warning {{The third argument is larger than the size of the input buffer. Replace with the value 'sizeof(dest)` or lower}} + strlcpy(dest, src, 20); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}} + strlcpy(dest, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}} strlcpy(dest, src, ulen); strlcpy(dest + 5, src, 5); - strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}} + strlcpy(dest + 5, src, 10); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof() or lower}} +} + +void testStrlcat(const char *src) { + char dest[10]; + size_t badlen = 20; + 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)); + strlcpy(dest, "0123456789", sizeof(dest)); + strlcpy(dest, "0123456789", sizeof(dest)); + strlcat(dest, "0123456789", badlen / 2); + strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) 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 sizeof() or lower}} }