Index: clang-tidy/cert/CERTTidyModule.cpp =================================================================== --- clang-tidy/cert/CERTTidyModule.cpp +++ clang-tidy/cert/CERTTidyModule.cpp @@ -20,6 +20,7 @@ #include "FloatLoopCounter.h" #include "SetLongJmpCheck.h" #include "StaticObjectExceptionCheck.h" +#include "StrToNumCheck.h" #include "ThrownExceptionTypeCheck.h" #include "VariadicFunctionDefCheck.h" @@ -64,6 +65,9 @@ // FIO CheckFactories.registerCheck( "cert-fio38-c"); + // ERR + CheckFactories.registerCheck( + "cert-err34-c"); } }; Index: clang-tidy/cert/CMakeLists.txt =================================================================== --- clang-tidy/cert/CMakeLists.txt +++ clang-tidy/cert/CMakeLists.txt @@ -6,10 +6,12 @@ FloatLoopCounter.cpp SetLongJmpCheck.cpp StaticObjectExceptionCheck.cpp + StrToNumCheck.cpp ThrownExceptionTypeCheck.cpp VariadicFunctionDefCheck.cpp LINK_LIBS + clangAnalysis clangAST clangASTMatchers clangBasic Index: clang-tidy/cert/StrToNumCheck.h =================================================================== --- clang-tidy/cert/StrToNumCheck.h +++ clang-tidy/cert/StrToNumCheck.h @@ -0,0 +1,36 @@ +//===--- StrToNumCheck.h - clang-tidy----------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_STRTONUMCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_STRTONUMCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace cert { + +/// Guards against use of string conversion functions that do not have +/// reasonable error handling for conversion errors. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/cert-err34-c.html +class StrToNumCheck : public ClangTidyCheck { +public: + StrToNumCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace cert +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_STRTONUMCHECK_H Index: clang-tidy/cert/StrToNumCheck.cpp =================================================================== --- clang-tidy/cert/StrToNumCheck.cpp +++ clang-tidy/cert/StrToNumCheck.cpp @@ -0,0 +1,235 @@ +//===--- Err34CCheck.cpp - clang-tidy--------------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "StrToNumCheck.h" +#include "clang/Analysis/Analyses/FormatString.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/StringSwitch.h" +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cert { + +void StrToNumCheck::registerMatchers(MatchFinder *Finder) { + // Match any function call to the C standard library string conversion + // functions that do no error checking. + Finder->addMatcher( + callExpr( + callee(functionDecl(anyOf( + functionDecl(hasAnyName("::atoi", "::atof", "::atol", "::atoll")) + .bind("converter"), + functionDecl(hasAnyName("::scanf", "::sscanf", "::fscanf", + "::vfscanf", "::vscanf", "::vsscanf")) + .bind("formatted"))))) + .bind("expr"), + this); +} + +namespace { +enum class ConversionKind { + None, + ToInt, + ToUInt, + ToLongInt, + ToLongUInt, + ToIntMax, + ToUIntMax, + ToFloat, + ToDouble, + ToLongDouble +}; + +ConversionKind ClassifyConversionFunc(const FunctionDecl *FD) { + return llvm::StringSwitch(FD->getName()) + .Cases("atoi", "atol", ConversionKind::ToInt) + .Case("atoll", ConversionKind::ToLongInt) + .Case("atof", ConversionKind::ToDouble) + .Default(ConversionKind::None); +} + +ConversionKind ClassifyFormatString(StringRef Fmt, const LangOptions &LO, + const TargetInfo &TI) { + // Scan the format string for the first problematic format specifier, then + // report that as the conversion type. This will miss additional conversion + // specifiers, but that is acceptable behavior. + + class Handler : public analyze_format_string::FormatStringHandler { + ConversionKind CK; + + bool HandleScanfSpecifier(const analyze_scanf::ScanfSpecifier &FS, + const char *startSpecifier, + unsigned specifierLen) override { + // If we just consume the argument without assignment, we don't care + // about it having conversion errors. + if (!FS.consumesDataArgument()) + return true; + + // Get the conversion specifier and use it to determine the conversion + // kind. + analyze_scanf::ScanfConversionSpecifier SCS = FS.getConversionSpecifier(); + if (SCS.isIntArg()) { + switch (FS.getLengthModifier().getKind()) { + case analyze_scanf::LengthModifier::AsLongLong: + CK = ConversionKind::ToLongInt; + break; + case analyze_scanf::LengthModifier::AsIntMax: + CK = ConversionKind::ToIntMax; + break; + default: + CK = ConversionKind::ToInt; + break; + } + } else if (SCS.isUIntArg()) { + switch (FS.getLengthModifier().getKind()) { + case analyze_scanf::LengthModifier::AsLongLong: + CK = ConversionKind::ToLongUInt; + break; + case analyze_scanf::LengthModifier::AsIntMax: + CK = ConversionKind::ToUIntMax; + break; + default: + CK = ConversionKind::ToUInt; + break; + } + } else if (SCS.isDoubleArg()) { + switch (FS.getLengthModifier().getKind()) { + case analyze_scanf::LengthModifier::AsLongDouble: + CK = ConversionKind::ToLongDouble; + break; + case analyze_scanf::LengthModifier::AsLong: + CK = ConversionKind::ToDouble; + break; + default: + CK = ConversionKind::ToFloat; + break; + } + } + + // Continue if we have yet to find a conversion kind that we care about. + return CK == ConversionKind::None; + } + + public: + Handler() : CK(ConversionKind::None) {} + + ConversionKind get() const { return CK; } + }; + + Handler H; + analyze_format_string::ParseScanfString(H, Fmt.begin(), Fmt.end(), LO, TI); + + return H.get(); +} + +const char *ClassifyConversionType(ConversionKind K) { + switch (K) { + case ConversionKind::None: + assert(false && "Unexpected conversion kind"); + case ConversionKind::ToInt: + case ConversionKind::ToLongInt: + case ConversionKind::ToIntMax: + return "an integer value"; + case ConversionKind::ToUInt: + case ConversionKind::ToLongUInt: + case ConversionKind::ToUIntMax: + return "an unsigned integer value"; + case ConversionKind::ToFloat: + case ConversionKind::ToDouble: + case ConversionKind::ToLongDouble: + return "a floating-point value"; + } + llvm_unreachable("Unknown conversion kind"); +} + +const char *ClassifyReplacement(ConversionKind K) { + switch (K) { + case ConversionKind::None: + assert(false && "Unexpected conversion kind"); + case ConversionKind::ToInt: + return "strtol"; + case ConversionKind::ToUInt: + return "strtoul"; + case ConversionKind::ToIntMax: + return "strtoimax"; + case ConversionKind::ToLongInt: + return "strtoll"; + case ConversionKind::ToLongUInt: + return "strtoull"; + case ConversionKind::ToUIntMax: + return "strtoumax"; + case ConversionKind::ToFloat: + return "strtof"; + case ConversionKind::ToDouble: + return "strtod"; + case ConversionKind::ToLongDouble: + return "strtold"; + } + llvm_unreachable("Unknown conversion kind"); +} +} // unnamed namespace + +void StrToNumCheck::check(const MatchFinder::MatchResult &Result) { + const auto *CE = Result.Nodes.getNodeAs("expr"); + const FunctionDecl *FD = nullptr; + ConversionKind CK; + + if (const auto *CFD = Result.Nodes.getNodeAs("converter")) { + FD = CFD; // Converter functions are always incorrect to use. + CK = ClassifyConversionFunc(CFD); + } else if (const auto *FFD = + Result.Nodes.getNodeAs("formatted")) { + StringRef FmtStr; + // The format string comes from the call expression and depends on which + // flavor of scanf is called. + // Index 0: scanf, vscanf, Index 1: fscanf, sscanf, vfscanf, vsscanf. + unsigned Idx = + (FFD->getName() == "scanf" || FFD->getName() == "vscanf") ? 0 : 1; + + // Given the index, see if the call expression argument at that index is + // a string literal. + if (CE->getNumArgs() < Idx) + return; + + if (const Expr *Arg = CE->getArg(Idx)->IgnoreParenImpCasts()) { + if (const auto *SL = dyn_cast(Arg)) { + FmtStr = SL->getString(); + } + } + + // If we could not get the format string, bail out. + if (FmtStr.empty()) + return; + + // Formatted input functions need further checking of the format string to + // determine whether a problematic conversion may be happening. + CK = ClassifyFormatString(FmtStr, Result.Context->getLangOpts(), + Result.Context->getTargetInfo()); + if (CK != ConversionKind::None) + FD = FFD; + } + + if (!FD) + return; + + std::string ConversionPerformed = ClassifyConversionType(CK), + AlternateFunction = ClassifyReplacement(CK); + + diag(CE->getExprLoc(), "%0 used to convert a string to %1, but function will " + "not report conversion errors; consider using '%2' " + "instead") + << FD << ConversionPerformed << AlternateFunction; +} + +} // namespace cert +} // namespace tidy +} // namespace clang Index: docs/clang-tidy/checks/cert-err34-c.rst =================================================================== --- docs/clang-tidy/checks/cert-err34-c.rst +++ docs/clang-tidy/checks/cert-err34-c.rst @@ -0,0 +1,13 @@ +.. title:: clang-tidy - cert-err34-c + +cert-err34-c +============ + +This check flags calls to string-to-number conversion functions that do not +verify the validity of the conversion, such as ``atoi()`` or ``scanf()``. It +does not flag calls to ``strtol()``, or other, related conversion functions that +do perform better error checking. + +This check corresponds to the CERT C Coding Standard rule +`ERR34-C. Detect errors when converting a string to a number +`_. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -9,6 +9,7 @@ cert-dcl54-cpp (redirects to misc-new-delete-overloads) cert-dcl59-cpp (redirects to google-build-namespaces) cert-env33-c + cert-err34-c cert-err52-cpp cert-err58-cpp cert-err60-cpp @@ -75,7 +76,7 @@ misc-string-literal-with-embedded-nul misc-suspicious-missing-comma misc-suspicious-semicolon - misc-suspicious-string-compare + misc-suspicious-string-compare misc-swapped-arguments misc-throw-by-value-catch-by-reference misc-undelegated-constructor Index: test/clang-tidy/cert-err34-c.c =================================================================== --- test/clang-tidy/cert-err34-c.c +++ test/clang-tidy/cert-err34-c.c @@ -0,0 +1,103 @@ +// RUN: %check_clang_tidy %s cert-err34-c %t -- -- -std=c11 + +typedef __SIZE_TYPE__ size_t; +typedef signed ptrdiff_t; +typedef long long intmax_t; +typedef unsigned long long uintmax_t; +typedef void * FILE; + +extern FILE *stdin; + +extern int fscanf(FILE * restrict stream, const char * restrict format, ...); +extern int scanf(const char * restrict format, ...); +extern int sscanf(const char * restrict s, const char * restrict format, ...); + +extern double atof(const char *nptr); +extern int atoi(const char *nptr); +extern long int atol(const char *nptr); +extern long long int atoll(const char *nptr); + +void f1(const char *in) { + int i; + long long ll; + unsigned int ui; + unsigned long long ull; + intmax_t im; + uintmax_t uim; + float f; + double d; + long double ld; + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'sscanf' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtol' instead [cert-err34-c] + sscanf(in, "%d", &i); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'fscanf' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtoll' instead [cert-err34-c] + fscanf(stdin, "%lld", &ll); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'sscanf' used to convert a string to an unsigned integer value, but function will not report conversion errors; consider using 'strtoul' instead [cert-err34-c] + sscanf(in, "%u", &ui); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'fscanf' used to convert a string to an unsigned integer value, but function will not report conversion errors; consider using 'strtoull' instead [cert-err34-c] + fscanf(stdin, "%llu", &ull); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtoimax' instead [cert-err34-c] + scanf("%jd", &im); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'fscanf' used to convert a string to an unsigned integer value, but function will not report conversion errors; consider using 'strtoumax' instead [cert-err34-c] + fscanf(stdin, "%ju", &uim); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'sscanf' used to convert a string to a floating-point value, but function will not report conversion errors; consider using 'strtof' instead [cert-err34-c] + sscanf(in, "%f", &f); // to float + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'fscanf' used to convert a string to a floating-point value, but function will not report conversion errors; consider using 'strtod' instead [cert-err34-c] + fscanf(stdin, "%lg", &d); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'sscanf' used to convert a string to a floating-point value, but function will not report conversion errors; consider using 'strtold' instead [cert-err34-c] + sscanf(in, "%Le", &ld); + + // These are conversions with other modifiers + short s; + char c; + size_t st; + ptrdiff_t pt; + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%hhd", &c); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%hd", &s); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%zu", &st); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%td", &pt); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%o", ui); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%X", ui); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%x", ui); +} + +void f2(const char *in) { + // CHECK-MESSAGES: :[[@LINE+1]]:11: warning: 'atoi' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtol' instead [cert-err34-c] + int i = atoi(in); // to int + // CHECK-MESSAGES: :[[@LINE+1]]:12: warning: 'atol' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtol' instead [cert-err34-c] + long l = atol(in); // to long + // CHECK-MESSAGES: :[[@LINE+1]]:18: warning: 'atoll' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtoll' instead [cert-err34-c] + long long ll = atoll(in); // to long long + // CHECK-MESSAGES: :[[@LINE+1]]:14: warning: 'atof' used to convert a string to a floating-point value, but function will not report conversion errors; consider using 'strtod' instead [cert-err34-c] + double d = atof(in); // to double +} + +void f3(void) { + int i; + unsigned int u; + float f; + char str[32]; + + // Test that we don't report multiple infractions for a single call. + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%d%u%f", &i, &u, &f); + + // Test that we still catch infractions that are not the first specifier. + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'scanf' used to convert + scanf("%s%d", str, &i); +} + +void do_not_diagnose(void) { + char str[32]; + + scanf("%s", str); // Not a numerical conversion + scanf("%*d"); // Assignment suppressed +} Index: test/clang-tidy/cert-err34-c.cpp =================================================================== --- test/clang-tidy/cert-err34-c.cpp +++ test/clang-tidy/cert-err34-c.cpp @@ -0,0 +1,43 @@ +// RUN: %check_clang_tidy %s cert-err34-c %t -- -- -std=c++11 + +typedef void * FILE; + +extern FILE *stdin; + +extern int fscanf(FILE * stream, const char * format, ...); +extern int sscanf(const char * s, const char * format, ...); + +extern double atof(const char *nptr); +extern int atoi(const char *nptr); +extern long int atol(const char *nptr); +extern long long int atoll(const char *nptr); + +namespace std { +using ::FILE; using ::stdin; +using ::fscanf; using ::sscanf; +using ::atof; using ::atoi; using ::atol; using ::atoll; +} + +void f1(const char *in) { + int i; + long long ll; + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'sscanf' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtol' instead [cert-err34-c] + std::sscanf(in, "%d", &i); + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: 'fscanf' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtoll' instead [cert-err34-c] + std::fscanf(std::stdin, "%lld", &ll); +} + +void f2(const char *in) { + // CHECK-MESSAGES: :[[@LINE+1]]:11: warning: 'atoi' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtol' instead [cert-err34-c] + int i = std::atoi(in); // to int + // CHECK-MESSAGES: :[[@LINE+1]]:12: warning: 'atol' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtol' instead [cert-err34-c] + long l = std::atol(in); // to long + + using namespace std; + + // CHECK-MESSAGES: :[[@LINE+1]]:18: warning: 'atoll' used to convert a string to an integer value, but function will not report conversion errors; consider using 'strtoll' instead [cert-err34-c] + long long ll = atoll(in); // to long long + // CHECK-MESSAGES: :[[@LINE+1]]:14: warning: 'atof' used to convert a string to a floating-point value, but function will not report conversion errors; consider using 'strtod' instead [cert-err34-c] + double d = atof(in); // to double +}