devnexen created this revision.
devnexen added reviewers: george.karpenkov, NoQ.
devnexen created this object with visibility "All Users".
Herald added a subscriber: cfe-commits.
- 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.
Repository:
rC Clang
https://reviews.llvm.org/D49722
Files:
lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
test/Analysis/cstring-syntax.c
Index: test/Analysis/cstring-syntax.c
===================================================================
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@
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 @@
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 <size> - 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(<destination buffer>) - 1 or lower}}
+}
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -92,6 +92,17 @@
/// 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 containsBadStrlcatPattern(const CallExpr *CE);
+
public:
WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
: Checker(Checker), BR(BR), AC(AC) {}
@@ -190,6 +201,57 @@
return false;
}
+bool WalkAST::containsBadStrlcatPattern(const CallExpr *CE) {
+ if (CE->getNumArgs() != 3)
+ return false;
+ const Expr *DstArg = CE->getArg(0);
+ const Expr *LenArg = CE->getArg(2);
+
+ const auto *DstArgDecl = dyn_cast<DeclRefExpr>(DstArg->IgnoreParenImpCasts());
+ const auto *LenArgDecl = dyn_cast<DeclRefExpr>(LenArg->IgnoreParenLValueCasts());
+ uint64_t DstOff = 0;
+ // - sizeof(dst)
+ if (isSizeof(LenArg, DstArg))
+ return true;
+ // - size_t dstlen = sizeof(dst)
+ if (LenArgDecl) {
+ const auto *LenArgVal = dyn_cast<VarDecl>(LenArgDecl->getDecl());
+ if (LenArgVal->getInit())
+ LenArg = LenArgVal->getInit();
+ }
+
+ // - integral value
+ // We try to figure out if the last argument is possibly longer or equal
+ // than the destination can possibly handle if its size can be defined.
+ if (const auto *IL = dyn_cast<IntegerLiteral>(LenArg->IgnoreParenImpCasts())) {
+ uint64_t ILRawVal = IL->getValue().getZExtValue();
+
+ // Case when there is pointer arithmetic on the destination buffer
+ // especially when we offset from the base decreasing the
+ // buffer length accordingly.
+ if (!DstArgDecl) {
+ if (const auto *BE = dyn_cast<BinaryOperator>(DstArg->IgnoreParenImpCasts())) {
+ DstArgDecl = dyn_cast<DeclRefExpr>(BE->getLHS()->IgnoreParenImpCasts());
+ if (BE->getOpcode() == BO_Add) {
+ if ((IL = dyn_cast<IntegerLiteral>(BE->getRHS()->IgnoreParenImpCasts()))) {
+ DstOff = IL->getValue().getZExtValue();
+ }
+ }
+ }
+ }
+ if (DstArgDecl) {
+ if (const auto *Buffer = dyn_cast<ConstantArrayType>(DstArgDecl->getType())) {
+ ASTContext &C = BR.getContext();
+ uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
+ if ((BufferLen - DstOff) <= ILRawVal)
+ return true;
+ }
+ }
+ }
+
+ return false;
+}
+
void WalkAST::VisitCallExpr(CallExpr *CE) {
const FunctionDecl *FD = CE->getDirectCallee();
if (!FD)
@@ -234,6 +296,34 @@
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 (containsBadStrlcatPattern(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 << " <size> ";
+ if (!DstName.empty())
+ os << " - strlen(" << DstName << ")";
+ else
+ os << " - strlen(<destination buffer>)";
+ os << " - 1 or lower";
+
BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
"C String API", os.str(), Loc,
LenArg->getSourceRange());
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
