[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
This commit is contained in:
parent
ee5e8ae845
commit
87b6ff09f9
|
@ -164,6 +164,8 @@ public:
|
||||||
/// Given a function declaration and a name checks if this is a C lib
|
/// Given a function declaration and a name checks if this is a C lib
|
||||||
/// function with the given name.
|
/// function with the given name.
|
||||||
bool isCLibraryFunction(const FunctionDecl *FD, StringRef 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
|
/// \brief Depending on wither the location corresponds to a macro, return
|
||||||
/// either the macro name or the token spelling.
|
/// either the macro name or the token spelling.
|
||||||
|
|
|
@ -14,6 +14,7 @@ add_clang_library(clangStaticAnalyzerCheckers
|
||||||
BasicObjCFoundationChecks.cpp
|
BasicObjCFoundationChecks.cpp
|
||||||
BuiltinFunctionChecker.cpp
|
BuiltinFunctionChecker.cpp
|
||||||
CStringChecker.cpp
|
CStringChecker.cpp
|
||||||
|
CStringSyntaxChecker.cpp
|
||||||
CallAndMessageChecker.cpp
|
CallAndMessageChecker.cpp
|
||||||
CastSizeChecker.cpp
|
CastSizeChecker.cpp
|
||||||
CastToStructChecker.cpp
|
CastToStructChecker.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<WalkAST> {
|
||||||
|
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<DeclRefExpr>(A1->IgnoreParenCasts()))
|
||||||
|
if (const DeclRefExpr *D2 = dyn_cast<DeclRefExpr>(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<UnaryExprOrTypeTraitExpr>(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<CallExpr>(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<IntegerLiteral>(E))
|
||||||
|
return (IL->getValue().isIntN(1));
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
inline StringRef getPrintableName(const Expr *E) {
|
||||||
|
if (const DeclRefExpr *D = dyn_cast<DeclRefExpr>(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<BinaryOperator>(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<check::ASTCodeBody> {
|
||||||
|
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<CStringSyntaxChecker>();
|
||||||
|
}
|
||||||
|
|
|
@ -33,6 +33,7 @@ def Taint : Package<"taint">, InPackage<SecurityExperimental>, Hidden;
|
||||||
|
|
||||||
def Unix : Package<"unix">;
|
def Unix : Package<"unix">;
|
||||||
def UnixExperimental : Package<"unix">, InPackage<Experimental>, Hidden;
|
def UnixExperimental : Package<"unix">, InPackage<Experimental>, Hidden;
|
||||||
|
def CString : Package<"cstring">, InPackage<UnixExperimental>, Hidden;
|
||||||
|
|
||||||
def OSX : Package<"osx">;
|
def OSX : Package<"osx">;
|
||||||
def OSXExperimental : Package<"osx">, InPackage<Experimental>, Hidden;
|
def OSXExperimental : Package<"osx">, InPackage<Experimental>, Hidden;
|
||||||
|
@ -281,10 +282,6 @@ def ChrootChecker : Checker<"Chroot">,
|
||||||
HelpText<"Check improper use of chroot">,
|
HelpText<"Check improper use of chroot">,
|
||||||
DescFile<"ChrootChecker.cpp">;
|
DescFile<"ChrootChecker.cpp">;
|
||||||
|
|
||||||
def CStringChecker : Checker<"CString">,
|
|
||||||
HelpText<"Check calls to functions in <string.h>">,
|
|
||||||
DescFile<"CStringChecker.cpp">;
|
|
||||||
|
|
||||||
def MallocChecker : Checker<"Malloc">,
|
def MallocChecker : Checker<"Malloc">,
|
||||||
HelpText<"Check for potential memory leaks, double free, and use-after-free problems">,
|
HelpText<"Check for potential memory leaks, double free, and use-after-free problems">,
|
||||||
DescFile<"MallocChecker.cpp">;
|
DescFile<"MallocChecker.cpp">;
|
||||||
|
@ -303,6 +300,17 @@ def StreamChecker : Checker<"Stream">,
|
||||||
|
|
||||||
} // end "unix.experimental"
|
} // end "unix.experimental"
|
||||||
|
|
||||||
|
let ParentPackage = CString in {
|
||||||
|
|
||||||
|
def CStringChecker : Checker<"Generic">,
|
||||||
|
HelpText<"Check calls to functions in <string.h>">,
|
||||||
|
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.
|
// Mac OS X, Cocoa, and Core Foundation checkers.
|
||||||
//===----------------------------------------------------------------------===//
|
//===----------------------------------------------------------------------===//
|
||||||
|
|
|
@ -37,19 +37,27 @@ StringRef CheckerContext::getCalleeName(const FunctionDecl *FunDecl) const {
|
||||||
|
|
||||||
|
|
||||||
bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
|
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
|
// To avoid false positives (Ex: finding user defined functions with
|
||||||
// similar names), only perform fuzzy name matching when it's a builtin.
|
// 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.
|
// Using a string compare is slow, we might want to switch on BuiltinID here.
|
||||||
unsigned BId = FD->getBuiltinID();
|
unsigned BId = FD->getBuiltinID();
|
||||||
if (BId != 0) {
|
if (BId != 0) {
|
||||||
ASTContext &Context = getASTContext();
|
|
||||||
StringRef BName = Context.BuiltinInfo.GetName(BId);
|
StringRef BName = Context.BuiltinInfo.GetName(BId);
|
||||||
if (StringRef(BName).find(Name) != StringRef::npos)
|
if (BName.find(Name) != StringRef::npos)
|
||||||
return true;
|
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 true;
|
||||||
|
|
||||||
return false;
|
return false;
|
||||||
|
|
|
@ -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 -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 -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 -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 -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
|
// Declarations
|
||||||
|
|
|
@ -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}}
|
||||||
|
}
|
|
@ -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 -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,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,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,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
|
// Declarations
|
||||||
|
|
Loading…
Reference in New Issue