Author: aaronballman Date: Fri Apr 29 15:56:48 2016 New Revision: 268100 URL: http://llvm.org/viewvc/llvm-project?rev=268100&view=rev Log: Add a clang-tidy check that flags string-to-number conversion functions that have insufficient error checking, suggesting a better alternative.
This check corresponds to: https://www.securecoding.cert.org/confluence/display/c/ERR34-C.+Detect+errors+when+converting+a+string+to+a+number Added: clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.cpp clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.h clang-tools-extra/trunk/docs/clang-tidy/checks/cert-err34-c.rst clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.c clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.cpp Modified: clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp clang-tools-extra/trunk/clang-tidy/cert/CMakeLists.txt clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Modified: clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp?rev=268100&r1=268099&r2=268100&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/cert/CERTTidyModule.cpp Fri Apr 29 15:56:48 2016 @@ -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 @@ public: // FIO CheckFactories.registerCheck<NonCopyableObjectsCheck>( "cert-fio38-c"); + // ERR + CheckFactories.registerCheck<StrToNumCheck>( + "cert-err34-c"); } }; Modified: clang-tools-extra/trunk/clang-tidy/cert/CMakeLists.txt URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cert/CMakeLists.txt?rev=268100&r1=268099&r2=268100&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/cert/CMakeLists.txt (original) +++ clang-tools-extra/trunk/clang-tidy/cert/CMakeLists.txt Fri Apr 29 15:56:48 2016 @@ -6,10 +6,12 @@ add_clang_library(clangTidyCERTModule FloatLoopCounter.cpp SetLongJmpCheck.cpp StaticObjectExceptionCheck.cpp + StrToNumCheck.cpp ThrownExceptionTypeCheck.cpp VariadicFunctionDefCheck.cpp LINK_LIBS + clangAnalysis clangAST clangASTMatchers clangBasic Added: clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.cpp?rev=268100&view=auto ============================================================================== --- clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.cpp (added) +++ clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.cpp Fri Apr 29 15:56:48 2016 @@ -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 <cassert> + +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<ConversionKind>(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(); +} + +StringRef 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"); +} + +StringRef 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 *Call = Result.Nodes.getNodeAs<CallExpr>("expr"); + const FunctionDecl *FuncDecl = nullptr; + ConversionKind Conversion; + + if (const auto *ConverterFunc = + Result.Nodes.getNodeAs<FunctionDecl>("converter")) { + // Converter functions are always incorrect to use. + FuncDecl = ConverterFunc; + Conversion = ClassifyConversionFunc(ConverterFunc); + } else if (const auto *FFD = + Result.Nodes.getNodeAs<FunctionDecl>("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 (Call->getNumArgs() < Idx) + return; + + if (const Expr *Arg = Call->getArg(Idx)->IgnoreParenImpCasts()) { + if (const auto *SL = dyn_cast<StringLiteral>(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. + Conversion = ClassifyFormatString(FmtStr, Result.Context->getLangOpts(), + Result.Context->getTargetInfo()); + if (Conversion != ConversionKind::None) + FuncDecl = FFD; + } + + if (!FuncDecl) + return; + + diag(Call->getExprLoc(), + "%0 used to convert a string to %1, but function will not report " + "conversion errors; consider using '%2' instead") + << FuncDecl << ClassifyConversionType(Conversion) + << ClassifyReplacement(Conversion); +} + +} // namespace cert +} // namespace tidy +} // namespace clang Added: clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.h?rev=268100&view=auto ============================================================================== --- clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.h (added) +++ clang-tools-extra/trunk/clang-tidy/cert/StrToNumCheck.h Fri Apr 29 15:56:48 2016 @@ -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 Added: clang-tools-extra/trunk/docs/clang-tidy/checks/cert-err34-c.rst URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/cert-err34-c.rst?rev=268100&view=auto ============================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/cert-err34-c.rst (added) +++ clang-tools-extra/trunk/docs/clang-tidy/checks/cert-err34-c.rst Fri Apr 29 15:56:48 2016 @@ -0,0 +1,28 @@ +.. 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. + +.. code:: c + + #include <stdlib.h> + + void func(const char *buff) { + int si; + + if (buff) { + si = atoi(buff); /* 'atoi' used to convert a string to an integer, but function will + not report conversion errors; consider using 'strtol' instead. */ + } else { + /* Handle error */ + } + } + +This check corresponds to the CERT C Coding Standard rule +`ERR34-C. Detect errors when converting a string to a number +<https://www.securecoding.cert.org/confluence/display/c/ERR34-C.+Detect+errors+when+converting+a+string+to+a+number>`_. Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=268100&r1=268099&r2=268100&view=diff ============================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original) +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Fri Apr 29 15:56:48 2016 @@ -11,6 +11,7 @@ Clang-Tidy Checks cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp> cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp> cert-env33-c + cert-err34-c cert-err52-cpp cert-err58-cpp cert-err60-cpp Added: clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.c URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.c?rev=268100&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.c (added) +++ clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.c Fri Apr 29 15:56:48 2016 @@ -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 +} Added: clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.cpp?rev=268100&view=auto ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.cpp (added) +++ clang-tools-extra/trunk/test/clang-tidy/cert-err34-c.cpp Fri Apr 29 15:56:48 2016 @@ -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 +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
