aaron.ballman created this revision.
aaron.ballman added reviewers: alexfh, sbenza.
aaron.ballman added a subscriber: cfe-commits.
Some numeric conversion APIs like atoi() and scanf() do not check the validity
of the value being converted, so it is impossible to tell whether range errors
have occurred. There are better APIs that can be used to ensure that input is
validated properly, such as strtol() and related functions. This clang-tidy
check flags calls to conversion functions that have insufficient error checking
and diagnoses them, 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.
http://reviews.llvm.org/D19590
Files:
clang-tidy/cert/CERTTidyModule.cpp
clang-tidy/cert/CMakeLists.txt
clang-tidy/cert/StrToNumCheck.cpp
clang-tidy/cert/StrToNumCheck.h
docs/clang-tidy/checks/cert-err34-c.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cert-err34-c.c
test/clang-tidy/cert-err34-c.cpp
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
+}
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: 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-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
@@ -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: 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
+<https://www.securecoding.cert.org/confluence/display/c/ERR34-C.+Detect+errors+when+converting+a+string+to+a+number>`_.
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 <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();
+}
+
+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<CallExpr>("expr");
+ const FunctionDecl *FD = nullptr;
+ ConversionKind CK;
+
+ if (const auto *CFD = Result.Nodes.getNodeAs<FunctionDecl>("converter")) {
+ FD = CFD; // Converter functions are always incorrect to use.
+ CK = ClassifyConversionFunc(CFD);
+ } 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 (CE->getNumArgs() < Idx)
+ return;
+
+ if (const Expr *Arg = CE->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.
+ 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: 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/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<NonCopyableObjectsCheck>(
"cert-fio38-c");
+ // ERR
+ CheckFactories.registerCheck<StrToNumCheck>(
+ "cert-err34-c");
}
};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
