From 87b6ff09f9e12d1255ed474cb8df6d93ab525eb2 Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Tue, 31 Jan 2012 19:33:39 +0000 Subject: [PATCH] [analyzer] Add checks for common anti-patterns in strncat. (Since this is syntax only, might be a good candidate for turning into a compiler warning.) llvm-svn: 149407 --- .../Core/PathSensitive/CheckerContext.h | 2 + .../StaticAnalyzer/Checkers/CMakeLists.txt | 1 + .../Checkers/CStringSyntaxChecker.cpp | 190 ++++++++++++++++++ clang/lib/StaticAnalyzer/Checkers/Checkers.td | 16 +- .../StaticAnalyzer/Core/CheckerContext.cpp | 16 +- clang/test/Analysis/bstring.c | 8 +- clang/test/Analysis/cstring-syntax.c | 13 ++ clang/test/Analysis/string.c | 8 +- 8 files changed, 238 insertions(+), 16 deletions(-) create mode 100644 clang/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp create mode 100644 clang/test/Analysis/cstring-syntax.c diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h index 71d8b3b7499f..0d8a80abd505 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -164,6 +164,8 @@ public: /// Given a function declaration and a name checks if this is a C lib /// function with the given name. bool isCLibraryFunction(const FunctionDecl *FD, StringRef Name); + static bool isCLibraryFunction(const FunctionDecl *FD, StringRef Name, + ASTContext &Context); /// \brief Depending on wither the location corresponds to a macro, return /// either the macro name or the token spelling. diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 9d628d40bb78..dc57a4cc3691 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -14,6 +14,7 @@ add_clang_library(clangStaticAnalyzerCheckers BasicObjCFoundationChecks.cpp BuiltinFunctionChecker.cpp CStringChecker.cpp + CStringSyntaxChecker.cpp CallAndMessageChecker.cpp CastSizeChecker.cpp CastToStructChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp new file mode 100644 index 000000000000..4f7643e88a01 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp @@ -0,0 +1,190 @@ +//== CStringSyntaxChecker.cpp - CoreFoundation containers API *- C++ -*-==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// An AST checker that looks for common pitfalls when using C string APIs. +// - Identifies erroneous patterns in the last argument to strncat - the number +// of bytes to copy. +// +//===----------------------------------------------------------------------===// +#include "ClangSACheckers.h" +#include "clang/Analysis/AnalysisContext.h" +#include "clang/AST/Expr.h" +#include "clang/AST/OperationKinds.h" +#include "clang/AST/StmtVisitor.h" +#include "clang/Basic/TargetInfo.h" +#include "clang/Basic/TypeTraits.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "llvm/Support/raw_ostream.h" + +using namespace clang; +using namespace ento; + +namespace { +class WalkAST: public StmtVisitor { + BugReporter &BR; + AnalysisDeclContext* AC; + ASTContext &ASTC; + + /// Check if two expressions refer to the same declaration. + inline bool sameDecl(const Expr *A1, const Expr *A2) { + if (const DeclRefExpr *D1 = dyn_cast(A1->IgnoreParenCasts())) + if (const DeclRefExpr *D2 = dyn_cast(A2->IgnoreParenCasts())) + return D1->getDecl() == D2->getDecl(); + return false; + } + + /// Check if the expression E is a sizeof(WithArg). + inline bool isSizeof(const Expr *E, const Expr *WithArg) { + if (const UnaryExprOrTypeTraitExpr *UE = + dyn_cast(E)) + if (UE->getKind() == UETT_SizeOf) + return sameDecl(UE->getArgumentExpr(), WithArg); + return false; + } + + /// Check if the expression E is a strlen(WithArg). + inline bool isStrlen(const Expr *E, const Expr *WithArg) { + if (const CallExpr *CE = dyn_cast(E)) { + const FunctionDecl *FD = CE->getDirectCallee(); + if (!FD) + return false; + return (CheckerContext::isCLibraryFunction(FD, "strlen", ASTC) + && sameDecl(CE->getArg(0), WithArg)); + } + return false; + } + + /// Check if the expression is an integer literal with value 1. + inline bool isOne(const Expr *E) { + if (const IntegerLiteral *IL = dyn_cast(E)) + return (IL->getValue().isIntN(1)); + return false; + } + + inline StringRef getPrintableName(const Expr *E) { + if (const DeclRefExpr *D = dyn_cast(E->IgnoreParenCasts())) + return D->getDecl()->getName(); + return StringRef(); + } + + /// Identify erroneous patterns in the last argument to strncat - the number + /// of bytes to copy. + bool containsBadStrncatPattern(const CallExpr *CE); + +public: + WalkAST(BugReporter &br, AnalysisDeclContext* ac) : + BR(br), AC(ac), ASTC(AC->getASTContext()) { + } + + // Statement visitor methods. + void VisitChildren(Stmt *S); + void VisitStmt(Stmt *S) { + VisitChildren(S); + } + void VisitCallExpr(CallExpr *CE); +}; +} // end anonymous namespace + +// The correct size argument should look like following: +// strncat(dst, src, sizeof(dst) - strlen(dest) - 1); +// We look for the following anti-patterns: +// - strncat(dst, src, sizeof(dst) - strlen(dst)); +// - strncat(dst, src, sizeof(dst) - 1); +// - strncat(dst, src, sizeof(dst)); +bool WalkAST::containsBadStrncatPattern(const CallExpr *CE) { + const Expr *DstArg = CE->getArg(0); + const Expr *SrcArg = CE->getArg(1); + const Expr *LenArg = CE->getArg(2); + + // Identify wrong size expressions, which are commonly used instead. + if (const BinaryOperator *BE = + dyn_cast(LenArg->IgnoreParenCasts())) { + // - sizeof(dst) - strlen(dst) + if (BE->getOpcode() == BO_Sub) { + const Expr *L = BE->getLHS(); + const Expr *R = BE->getRHS(); + if (isSizeof(L, DstArg) && isStrlen(R, DstArg)) + return true; + + // - sizeof(dst) - 1 + if (isSizeof(L, DstArg) && isOne(R->IgnoreParenCasts())) + return true; + } + } + // - sizeof(dst) + if (isSizeof(LenArg, DstArg)) + return true; + + // - sizeof(src) + if (isSizeof(LenArg, SrcArg)) + return true; + return false; +} + +void WalkAST::VisitCallExpr(CallExpr *CE) { + const FunctionDecl *FD = CE->getDirectCallee(); + if (!FD) + return; + + if (CheckerContext::isCLibraryFunction(FD, "strncat", ASTC)) { + if (containsBadStrncatPattern(CE)) { + const Expr *DstArg = CE->getArg(0); + const Expr *LenArg = CE->getArg(2); + SourceRange R = LenArg->getSourceRange(); + PathDiagnosticLocation Loc = + PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), AC); + + StringRef DstName = getPrintableName(DstArg); + + llvm::SmallString<256> S; + llvm::raw_svector_ostream os(S); + os << "Potential buffer overflow. "; + if (!DstName.empty()) { + os << "Replace with 'sizeof(" << DstName << ") " + "- strlen(" << DstName <<") - 1'"; + os << " or u"; + } else + os << "U"; + os << "se a safer 'strlcat' API"; + + BR.EmitBasicReport("Anti-pattern in the argument", "C String API", + os.str(), Loc, &R, 1); + } + } + + // Recurse and check children. + VisitChildren(CE); +} + +void WalkAST::VisitChildren(Stmt *S) { + for (Stmt::child_iterator I = S->child_begin(), E = S->child_end(); I != E; + ++I) + if (Stmt *child = *I) + Visit(child); +} + +namespace { +class CStringSyntaxChecker: public Checker { +public: + + void checkASTCodeBody(const Decl *D, AnalysisManager& Mgr, + BugReporter &BR) const { + WalkAST walker(BR, Mgr.getAnalysisDeclContext(D)); + walker.Visit(D->getBody()); + } +}; +} + +void ento::registerCStringSyntaxChecker(CheckerManager &mgr) { + mgr.registerChecker(); +} + diff --git a/clang/lib/StaticAnalyzer/Checkers/Checkers.td b/clang/lib/StaticAnalyzer/Checkers/Checkers.td index b713d5d331b0..7bf865ffcbb7 100644 --- a/clang/lib/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/lib/StaticAnalyzer/Checkers/Checkers.td @@ -33,6 +33,7 @@ def Taint : Package<"taint">, InPackage, Hidden; def Unix : Package<"unix">; def UnixExperimental : Package<"unix">, InPackage, Hidden; +def CString : Package<"cstring">, InPackage, Hidden; def OSX : Package<"osx">; def OSXExperimental : Package<"osx">, InPackage, Hidden; @@ -281,10 +282,6 @@ def ChrootChecker : Checker<"Chroot">, HelpText<"Check improper use of chroot">, DescFile<"ChrootChecker.cpp">; -def CStringChecker : Checker<"CString">, - HelpText<"Check calls to functions in ">, - DescFile<"CStringChecker.cpp">; - def MallocChecker : Checker<"Malloc">, HelpText<"Check for potential memory leaks, double free, and use-after-free problems">, DescFile<"MallocChecker.cpp">; @@ -303,6 +300,17 @@ def StreamChecker : Checker<"Stream">, } // end "unix.experimental" +let ParentPackage = CString in { + +def CStringChecker : Checker<"Generic">, + HelpText<"Check calls to functions in ">, + DescFile<"CStringChecker.cpp">; + +def CStringSyntaxChecker : Checker<"BadSizeArg">, + HelpText<"Check the size argument passed into C string functions for common erroneous patterns">, + DescFile<"CStringSyntaxChecker.cpp">; +} + //===----------------------------------------------------------------------===// // Mac OS X, Cocoa, and Core Foundation checkers. //===----------------------------------------------------------------------===// diff --git a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp index c25ba36a5126..ccf415f0c71a 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp @@ -37,19 +37,27 @@ StringRef CheckerContext::getCalleeName(const FunctionDecl *FunDecl) const { bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD, - StringRef Name){ + StringRef Name) { + return isCLibraryFunction(FD, Name, getASTContext()); +} + +bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD, + StringRef Name, ASTContext &Context) { // To avoid false positives (Ex: finding user defined functions with // similar names), only perform fuzzy name matching when it's a builtin. // Using a string compare is slow, we might want to switch on BuiltinID here. unsigned BId = FD->getBuiltinID(); if (BId != 0) { - ASTContext &Context = getASTContext(); StringRef BName = Context.BuiltinInfo.GetName(BId); - if (StringRef(BName).find(Name) != StringRef::npos) + if (BName.find(Name) != StringRef::npos) return true; } - if (FD->isExternC() && FD->getIdentifier()->getName().equals(Name)) + StringRef FName = FD->getIdentifier()->getName(); + if (FName.startswith("__inline")) + return (FName.find(Name) != StringRef::npos); + + if (FD->isExternC() && FName.equals(Name)) return true; return false; diff --git a/clang/test/Analysis/bstring.c b/clang/test/Analysis/bstring.c index e23059fcec48..8cdbb18b3577 100644 --- a/clang/test/Analysis/bstring.c +++ b/clang/test/Analysis/bstring.c @@ -1,7 +1,7 @@ -// RUN: %clang_cc1 -analyze -analyzer-checker=core,experimental.unix.CString -analyzer-store=region -Wno-null-dereference -verify %s -// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -analyzer-checker=core,experimental.unix.CString -analyzer-store=region -Wno-null-dereference -verify %s -// RUN: %clang_cc1 -analyze -DVARIANT -analyzer-checker=core,experimental.unix.CString -analyzer-store=region -Wno-null-dereference -verify %s -// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -DVARIANT -analyzer-checker=core,experimental.unix.CString -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -analyzer-checker=core,experimental.unix.cstring.Generic -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -analyzer-checker=core,experimental.unix.cstring.Generic -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -DVARIANT -analyzer-checker=core,experimental.unix.cstring.Generic -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -DVARIANT -analyzer-checker=core,experimental.unix.cstring.Generic -analyzer-store=region -Wno-null-dereference -verify %s //===----------------------------------------------------------------------=== // Declarations diff --git a/clang/test/Analysis/cstring-syntax.c b/clang/test/Analysis/cstring-syntax.c new file mode 100644 index 000000000000..b6e898bcc823 --- /dev/null +++ b/clang/test/Analysis/cstring-syntax.c @@ -0,0 +1,13 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=experimental.unix.cstring.BadSizeArg -analyzer-store=region -Wno-strlcpy-strlcat-size -Wno-sizeof-array-argument -Wno-sizeof-pointer-memaccess -verify %s + +typedef __SIZE_TYPE__ size_t; +char *strncat(char *, const char *, size_t); +size_t strlen (const char *s); + +void testStrncat(const char *src) { + char dest[10]; + strncat(dest, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAA", sizeof(dest) - 1); // expected-warning {{Potential buffer overflow. Replace with 'sizeof(dest) - strlen(dest) - 1' or use a safer 'strlcat' API}} + strncat(dest, "AAAAAAAAAAAAAAAAAAAAAAAAAAA", sizeof(dest)); // expected-warning {{Potential buffer overflow. Replace with}} + strncat(dest, "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", sizeof(dest) - strlen(dest)); // expected-warning {{Potential buffer overflow. Replace with}} + strncat(dest, src, sizeof(src)); // expected-warning {{Potential buffer overflow. Replace with}} +} diff --git a/clang/test/Analysis/string.c b/clang/test/Analysis/string.c index d72abb349a5e..9b0dea3d6cc4 100644 --- a/clang/test/Analysis/string.c +++ b/clang/test/Analysis/string.c @@ -1,7 +1,7 @@ -// RUN: %clang_cc1 -analyze -analyzer-checker=core,experimental.unix.CString,experimental.deadcode.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s -// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -analyzer-checker=core,experimental.unix.CString,experimental.deadcode.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s -// RUN: %clang_cc1 -analyze -DVARIANT -analyzer-checker=core,experimental.unix.CString,experimental.deadcode.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s -// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -DVARIANT -analyzer-checker=experimental.security.taint,core,experimental.unix.CString,experimental.deadcode.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -analyzer-checker=core,experimental.unix.cstring.Generic,experimental.deadcode.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -analyzer-checker=core,experimental.unix.cstring.Generic,experimental.deadcode.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -DVARIANT -analyzer-checker=core,experimental.unix.cstring.Generic,experimental.deadcode.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -DVARIANT -analyzer-checker=experimental.security.taint,core,experimental.unix.cstring.Generic,experimental.deadcode.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s //===----------------------------------------------------------------------=== // Declarations