Index: include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- include/clang/StaticAnalyzer/Checkers/Checkers.td +++ include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -144,6 +144,10 @@ HelpText<"Warn about assigning non-{0,1} values to Boolean variables">, DescFile<"BoolAssignmentChecker.cpp">; +def CallArgsOrderChecker : Checker<"CallArgsOrder">, + HelpText<"Warn about accidental args swap in various call expressions">, + DescFile<"CallArgsOrderChecker.cpp">; + def CastSizeChecker : Checker<"CastSize">, HelpText<"Check when casting a malloc'ed type T, whether the size is a multiple of the size of T">, DescFile<"CastSizeChecker.cpp">; Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -15,6 +15,7 @@ CStringChecker.cpp CStringSyntaxChecker.cpp CallAndMessageChecker.cpp + CallArgsOrderChecker.cpp CastSizeChecker.cpp CastToStructChecker.cpp CheckObjCDealloc.cpp Index: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp +++ lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp @@ -0,0 +1,431 @@ +//= CallArgsOrderChecker.cpp - Check for arguments order in calls -*- C++ -*-=// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines CallArgsOrderChecker, which looks for accidental swap or skip +// of arguments in function, methods, constructors and operators calls. +// +//===----------------------------------------------------------------------===// + +#include "clang/AST/ExprCXX.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { +constexpr unsigned TypicalMaxParamsCount = 4; +typedef SmallVector StringRefVec; + +class ParamsGroup { +public: + ParamsGroup() : StartIndex(0) { ParamsVec.reserve(4); } + + static StringRefVec trimSame(StringRefVec StrVec); + static StringRefVec ltrimSame(StringRefVec StrVec); + static StringRefVec rtrimSame(StringRefVec StrVec); + + void fillSimplifiedParamNames() { + StringRefVec ParamNames; + ParamNames.reserve(ParamsVec.size()); + for (const Param &P : ParamsVec) + ParamNames.push_back(P.Decl->getName()); + + if (ParamNames.size() > 1) + ParamNames = trimSame(ParamNames); + + for (StringRefVec::size_type i = 0, N = ParamsVec.size(); i < N; i++) + ParamsVec[i].SimplifiedName = ParamNames[i]; + } + + class Param { + public: + const ParmVarDecl *Decl; + StringRef SimplifiedName; + + bool isNamedLike(StringRef Name) const { + if (SimplifiedName.size() < Name.size()) { + if (Name.startswith_lower(SimplifiedName)) + return true; + } else { + if (SimplifiedName.startswith_lower(Name)) + return true; + } + + // We don't like false-positive matches on single-char + if (SimplifiedName.size() < 2 || Name.size() < 2) + return false; + + if (SimplifiedName.size() < Name.size()) + return Name.endswith_lower(SimplifiedName); + return SimplifiedName.endswith_lower(Name); + } + }; + + unsigned StartIndex; + SmallVector ParamsVec; +}; + +typedef llvm::SmallVector ParamsGroupVec; + +class CallArgsOrderChecker : public Checker { + mutable llvm::DenseMap PGLCache; + +public: + const ParamsGroupVec &getOrBuildParamsGroups(const FunctionDecl *FD) const; + void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr, + BugReporter &BR) const; +}; + +class Callback : public MatchFinder::MatchCallback { + const CallArgsOrderChecker *C; + BugReporter &BR; + SourceManager &SM; + AnalysisDeclContext *AC; + + void checkCallExpr(const CallExpr *CE); + void checkCXXConstructExpr(const CXXConstructExpr *CE); + + template + void checkCall(const FunctionDecl *FD, const CallExprT *CE, + unsigned FirstActualArgOffset) const; + +public: + Callback(const CallArgsOrderChecker *C, BugReporter &BR, SourceManager &SM, + AnalysisDeclContext *AC) + : C(C), BR(BR), SM(SM), AC(AC) {} + + virtual void run(const MatchFinder::MatchResult &Result) override { + if (const CallExpr *CE = Result.Nodes.getNodeAs("callExpr")) + checkCallExpr(CE); + if (const CXXConstructExpr *CE = + Result.Nodes.getNodeAs("cxxConstructExpr")) + checkCXXConstructExpr(CE); + } +}; +} // end of anonymous namespace + +StringRefVec ParamsGroup::trimSame(StringRefVec StrVec) { + return rtrimSame(ltrimSame(StrVec)); +} + +StringRefVec ParamsGroup::ltrimSame(StringRefVec StrVec) { + if (StrVec.empty()) + return StrVec; + + auto It = StrVec.begin(); + const auto End = StrVec.end(); + + StringRef SamePrefix = *It; + It++; + + for (; It != End; It++) { + const StringRef &Str = *It; + + const size_t StrSize = Str.size(); + size_t SamePrefixSize = SamePrefix.size(); + + if (SamePrefixSize > StrSize) { + SamePrefix = SamePrefix.drop_back(SamePrefixSize - StrSize); + SamePrefixSize = SamePrefix.size(); + } + + for (size_t i = 0; i < SamePrefixSize; i++) { + if (Str[i] != SamePrefix[i]) { + SamePrefix = SamePrefix.drop_back(SamePrefixSize - i); + break; + } + } + + // No common prefix found + if (SamePrefix.empty()) + return StrVec; + } + + const size_t SamePrefixSize = SamePrefix.size(); + std::transform(StrVec.begin(), StrVec.end(), StrVec.begin(), + [SamePrefixSize](const StringRef &Str) { + return Str.drop_front(SamePrefixSize); + }); + return StrVec; +} + +StringRefVec ParamsGroup::rtrimSame(StringRefVec StrVec) { + if (StrVec.empty()) + return StrVec; + + auto It = StrVec.begin(); + const auto End = StrVec.end(); + + StringRef SameSuffix = *It; + It++; + + for (; It != End; It++) { + const StringRef &Str = *It; + + const size_t StrSize = Str.size(); + size_t SameSuffixSize = SameSuffix.size(); + + if (SameSuffixSize > StrSize) { + SameSuffix = SameSuffix.drop_front(SameSuffixSize - StrSize); + SameSuffixSize = SameSuffix.size(); + } + + for (size_t i = StrSize, j = SameSuffixSize; i > 0 && j > 0; i--, j--) { + if (Str[i - 1] != SameSuffix[j - 1]) { + SameSuffix = SameSuffix.drop_front(j); + break; + } + } + + // No common suffix found + if (SameSuffix.empty()) + return StrVec; + } + + const size_t SameSuffixSize = SameSuffix.size(); + std::transform(StrVec.begin(), StrVec.end(), StrVec.begin(), + [SameSuffixSize](const StringRef &Str) { + return Str.drop_back(SameSuffixSize); + }); + return StrVec; +} + +// Split function params in groups with same qualified types +const ParamsGroupVec & +CallArgsOrderChecker::getOrBuildParamsGroups(const FunctionDecl *FD) const { + const auto It = PGLCache.find(FD); + if (It != PGLCache.end()) + return It->second; + + ParamsGroupVec PGL; + + const unsigned NumParams = FD->getNumParams(); + if (NumParams > 0) { + const ParmVarDecl *prevParam = FD->getParamDecl(0); + ParamsGroup PG; + + ParamsGroup::Param p; + p.Decl = prevParam; + + PG.ParamsVec.push_back(std::move(p)); + + PGL.push_back(std::move(PG)); + + for (unsigned ParamIndex = 1; ParamIndex < NumParams; ParamIndex++) { + const ParmVarDecl *currParam = FD->getParamDecl(ParamIndex); + + QualType prevType = prevParam->getOriginalType().getCanonicalType(); + QualType currType = currParam->getOriginalType().getCanonicalType(); + + ParamsGroup::Param p; + p.Decl = currParam; + + if (currType == prevType) { + PGL.back().ParamsVec.push_back(std::move(p)); + } else { + ParamsGroup PG; + PG.StartIndex = ParamIndex; + + PG.ParamsVec.push_back(std::move(p)); + + PGL.push_back(PG); + } + + prevParam = currParam; + } + + for (ParamsGroup &PG : PGL) + PG.fillSimplifiedParamNames(); + } + + auto InsertRes = PGLCache.insert(std::make_pair(FD, std::move(PGL))); + return InsertRes.first->second; +} + +static StringRef getExprName(const Expr *E) { + const Expr *OrigE = E->IgnoreParenCasts(); + if (!OrigE) + return StringRef(); + + if (const DeclRefExpr *DRE = dyn_cast_or_null(OrigE)) + return DRE->getFoundDecl()->getName(); + else if (const UnaryOperator *UO = dyn_cast_or_null(OrigE)) + return getExprName(UO->getSubExpr()); + else if (const CallExpr *CE = dyn_cast_or_null(OrigE)) + return getExprName(CE->getCallee()); + else if (const MemberExpr *ME = dyn_cast_or_null(OrigE)) + return ME->getMemberDecl()->getName(); + + return StringRef(); +} + +static StringRef getExprString(const Expr *E, SourceManager &SM) { + SourceLocation Start(E->getLocStart()); + SourceLocation End(E->getLocEnd()); + + End = Lexer::getLocForEndOfToken(End, 0, SM, LangOptions()); + const char *Str = SM.getCharacterData(Start); + return StringRef(Str, SM.getCharacterData(End) - Str); +} + +template +static unsigned getDefaultArgsCount(const CallExprT *CE) { + const unsigned NumArgs = CE->getNumArgs(); + for (unsigned ArgIndex = 0; ArgIndex < NumArgs; ArgIndex++) + if (isa(CE->getArg(ArgIndex))) + return NumArgs - ArgIndex; + return 0; +} + +void Callback::checkCallExpr(const CallExpr *CE) { + unsigned FirstActualArgOffset = 0; + if (const CXXOperatorCallExpr *OCE = + dyn_cast_or_null(CE)) { + if (OCE->getOperator() != OO_Call) + return; + + // Skipping first implicit object argument + FirstActualArgOffset = 1; + } + + const FunctionDecl *FD = CE->getDirectCallee(); + if (!FD) + return; + + checkCall(FD, CE, FirstActualArgOffset); +} + +void Callback::checkCXXConstructExpr(const CXXConstructExpr *CE) { + const FunctionDecl *FD = CE->getConstructor(); + if (!FD) + return; + + checkCall(FD, CE, /*FirstActualArgOffset=*/0); +} + +template +void Callback::checkCall(const FunctionDecl *FD, const CallExprT *CE, + unsigned FirstActualArgOffset) const { + // We are not interested in calls without arguments + const unsigned NumArgs = CE->getNumArgs(); + if (NumArgs <= FirstActualArgOffset) + return; + + const unsigned NumParams = FD->getNumParams(); + // Args order permutation isn't possible if FD has < 2 params + if (NumParams <= 1) + return; + + const unsigned DefNumArgs = getDefaultArgsCount(CE); + + const ParamsGroupVec &PGV = C->getOrBuildParamsGroups(FD); + for (ParamsGroupVec::size_type PGVIndex = 0, PGVSize = PGV.size(); + PGVIndex < PGVSize; PGVIndex++) { + const ParamsGroup &PG = PGV[PGVIndex]; + + const unsigned PVSize = PG.ParamsVec.size(); + if (PVSize < 2) + continue; + + StringRefVec SimplifiedArgNames; + SimplifiedArgNames.reserve(PVSize); + for (unsigned PVIndex = 0; PVIndex < PVSize; PVIndex++) { + const unsigned ArgIndex = PG.StartIndex + PVIndex + FirstActualArgOffset; + SimplifiedArgNames.push_back(getExprName(CE->getArg(ArgIndex))); + } + + SimplifiedArgNames = ParamsGroup::trimSame(SimplifiedArgNames); + + const unsigned InvalidMismatchIndex = ~unsigned(0); + unsigned FirstMismatchIndex = InvalidMismatchIndex; + + for (unsigned PVIndex = 0; PVIndex < PVSize - 1; PVIndex++) { + const ParamsGroup::Param &CurrParam = PG.ParamsVec[PVIndex]; + const ParamsGroup::Param &NextParam = PG.ParamsVec[PVIndex + 1]; + + StringRef CurrArgExprName = SimplifiedArgNames[PVIndex]; + StringRef NextArgExprName = SimplifiedArgNames[PVIndex + 1]; + + // Skip invalid simplified names + if (CurrParam.SimplifiedName.empty() || CurrArgExprName.empty()) + return; + + // Looks like arg matches param + if (CurrParam.isNamedLike(CurrArgExprName)) + continue; + + const unsigned ArgIndex = PG.StartIndex + PVIndex + FirstActualArgOffset; + const Expr *CurrArg = CE->getArg(ArgIndex); + const Expr *NextArg = CE->getArg(ArgIndex + 1); + + if (FirstMismatchIndex == InvalidMismatchIndex) + FirstMismatchIndex = PG.StartIndex + PVIndex; + + if (NextParam.isNamedLike(CurrArgExprName)) { + PathDiagnosticLocation Loc = + PathDiagnosticLocation::createBegin(CurrArg, SM, AC); + + SmallString<128> Message; + llvm::raw_svector_ostream OS(Message); + + // Looks like arg for CurrParam is missing + // and args sequence is shifted towards the left + if (DefNumArgs > 0 && PG.StartIndex + PVIndex + 1 == NumParams - 1) { + OS << "possibly missing arg for param '" + << FD->getParamDecl(FirstMismatchIndex)->getName() << "', arg '" + << getExprString(CE->getArg(FirstMismatchIndex), SM) + << "' passed instead"; + BR.EmitBasicReport(AC->getDecl(), C, "Possibly missing argument", + categories::LogicError, Message, Loc, + CurrArg->getSourceRange()); + break; + // Looks like args are swapped + } else if (CurrParam.isNamedLike(NextArgExprName)) { + OS << "possibly wrong order of args '" << getExprString(CurrArg, SM) + << "' and '" << getExprString(NextArg, SM) + << "' passed as params '" << CurrParam.Decl->getName() << "' and '" + << NextParam.Decl->getName() << "'"; + SourceRange Sr[2]; + Sr[0] = CurrArg->getSourceRange(); + Sr[1] = NextArg->getSourceRange(); + BR.EmitBasicReport(AC->getDecl(), C, + "Possibly wrong order of arguments", + categories::LogicError, Message, Loc, Sr); + } + } + } + } +} + +void CallArgsOrderChecker::checkASTCodeBody(const Decl *D, AnalysisManager &Mgr, + BugReporter &BR) const { + auto CallExpr = stmt( + eachOf(forEachDescendant(callExpr().bind("callExpr")), + forEachDescendant(cxxConstructExpr().bind("cxxConstructExpr")))); + + MatchFinder F; + Callback CB(this, BR, Mgr.getSourceManager(), Mgr.getAnalysisDeclContext(D)); + + F.addMatcher(CallExpr, &CB); + F.match(*D->getBody(), Mgr.getASTContext()); +} + +namespace clang { +namespace ento { +void registerCallArgsOrderChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} +} // namespace ento +} // namespace clang Index: test/Analysis/call-args-order.c =================================================================== --- test/Analysis/call-args-order.c +++ test/Analysis/call-args-order.c @@ -0,0 +1,57 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.CallArgsOrder -verify -std=c99 %s + +struct Viewport { + int width; + int height; + + float aspectRatio; +}; + +void setDimentions(struct Viewport *vp, int width, int height) { + vp->width = width; + vp->height = height; + vp->aspectRatio = width / height; +} + +float getWidth(struct Viewport *vp) { return vp->width; } +float getHeight(struct Viewport *vp) { return vp->height; } + +float getFloat() { + return 128.5; +} + +int main() { + struct Viewport vp; + + int newWidth = 800; + int newHeight = 600; + setDimentions(&vp, newHeight, newWidth); // expected-warning {{possibly wrong order of args 'newHeight' and 'newWidth' passed as params 'width' and 'height'}} + setDimentions(&vp, newWidth, newHeight); // no-warning + setDimentions(&vp, newWidth, newWidth); // no-warning + setDimentions(&vp, newHeight, newHeight); // no-warning + setDimentions(&vp, getFloat(), getFloat()); // no-warning + + int w = 640; + int h = 480; + setDimentions(&vp, w, h); // no-warning + setDimentions(&vp, h, w); // expected-warning {{possibly wrong order of args 'h' and 'w' passed as params 'width' and 'height'}} + setDimentions(&vp, w, w); // no-warning + setDimentions(&vp, h, h); // no-warning + + int w_big = 1920; + int h_big = 1080; + + float w_big_f = 320.0; + float h_big_f = 240.0; + + setDimentions(&vp, w_big, h_big); // no-warning + setDimentions(&vp, h_big, w_big); // expected-warning {{possibly wrong order of args 'h_big' and 'w_big' passed as params 'width' and 'height'}} + setDimentions(&vp, w_big, w_big); // no-warning + setDimentions(&vp, h_big, h_big); // no-warning + + setDimentions(&vp, (int)h_big_f, -w_big_f); // expected-warning {{possibly wrong order of args '(int)h_big_f' and '-w_big_f' passed as params 'width' and 'height'}} + setDimentions(&vp, (int)h_big_f, h_big_f); // no-warning + setDimentions(&vp, -getHeight(&vp), getWidth(&vp)); // expected-warning {{possibly wrong order of args '-getHeight(&vp)' and 'getWidth(&vp)' passed as params 'width' and 'height'}} + setDimentions(&vp, -getHeight(&vp), getHeight(&vp)); // no-warning + setDimentions(&vp, getHeight(&vp) + 4, getWidth(&vp)); // no-warning +} Index: test/Analysis/call-args-order.cpp =================================================================== --- test/Analysis/call-args-order.cpp +++ test/Analysis/call-args-order.cpp @@ -0,0 +1,103 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.CallArgsOrder -verify -x c++ %s + +typedef unsigned char byte; + +class Color { + byte r, g, b, a; + +public: + Color(byte R, byte G, byte B, byte A = 0) + : r(R), g(G), b(B), a(A) {} + + void set(byte R, byte G, byte B, byte A = 0) { + r = R; + g = G; + b = B; + a = A; + } + + void operator()(byte R, byte G, byte B, byte A = 0); + void operator+=(byte Delta); + + byte getR() const { return r; } + byte getG() const { return g; } + byte getB() const { return b; } + byte getA() const { return a; } + + static Color copy1(const Color &Color); + static Color copy2(const Color &Color); + static Color copy3(const Color &Color); + static Color copy4(const Color &Color); + static Color copy5(const Color &Color); + static Color copy6(const Color &Color); +}; + +void Color::operator()(byte R, byte G, byte B, byte A) { + set(R, B, G, A); // expected-warning {{possibly wrong order of args 'B' and 'G' passed as params 'G' and 'B'}} +} + +void Color::operator+=(byte Delta) { + r += Delta; + g += Delta; + b += Delta; + a += Delta; +} + +Color newRed1(byte r, byte b, byte a) { + return Color(r, b, a); // expected-warning {{possibly missing arg for param 'G'}} +} + +Color newRed2(byte r, byte g, byte a) { + return Color(r, g, a); // expected-warning {{possibly missing arg for param 'B'}} +} + +Color Color::copy1(const Color &o) { + return Color(o.r, o.b, o.g, o.a); // expected-warning {{possibly wrong order of args 'o.b' and 'o.g' passed as params 'G' and 'B'}} +} + +Color Color::copy2(const Color &o) { + return Color(o.r, o.g, o.a, o.b); // expected-warning {{possibly wrong order of args 'o.a' and 'o.b' passed as params 'B' and 'A'}} +} + +Color Color::copy3(const Color &o) { + return Color(o.r, o.g, o.g, o.g); // no-warning +} + +Color Color::copy4(const Color &o) { + return Color(o.r, o.r, o.r, o.r); // no-warning +} + +Color Color::copy5(const Color &o) { + return Color(o.getR(), o.getB(), o.getG(), o.getA()); // expected-warning {{possibly wrong order of args 'o.getB()' and 'o.getG()' passed as params 'G' and 'B'}} +} + +Color Color::copy6(const Color &o) { + // totaly messed up (not sure if it's wrong or not) + return Color(o.a, o.r, o.g, o.b); // no-warning +} + +Color newMagicColor() { + const byte r = 200; + const byte g = 150; + const byte b = 100; + const byte a = 0; + + return Color(r, b, g, a); // expected-warning {{possibly wrong order of args 'b' and 'g' passed as params 'G' and 'B'}} +} + +void modify(Color &c) { + int red = 1; + int green = 2; + int blue = 3; + c.set(red, green, blue); // no-warning + c.set(red, blue, green); // expected-warning {{possibly wrong order of args 'blue' and 'green' passed as params 'G' and 'B'}} + c.set(red, green, 0); // no-warning + c.set(red, green, c.getA()); // no-warning + + c(red, green, blue); // no-warning + c(green, red, blue); // expected-warning {{possibly wrong order of args 'green' and 'red' passed as params 'R' and 'G'}} + + c += 1; // no-warning + + c(red, blue, green, 0); // expected-warning {{possibly wrong order of args 'blue' and 'green' passed as params 'G' and 'B'}} +}