Check for CERT ERR34-C. Detect errors when converting a string to a number

[aaron.ballman]

Summary

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.

[alexfh]

Looks good with a few nits.

clang-tidy/cert/StrToNumCheck.cpp

182

Too many abbreviations for my taste. How about CE -> Call, FD -> Function or FuncDecl, CK -> Conversion, CFD -> ConverterFunc?

224

1. s/std::string/StringRef/
2. I'd make the functions return StringRef instead of a const char *
3. One variable per declaration, please.
4. I'm not sure the variables help making the code easier to read.

docs/clang-tidy/checks/cert-err34-c.rst

9

Maybe add a couple of examples?

[aaron.ballman]

Thanks! I've commit in r268100 with the changes you requested.

clang-tidy/cert/StrToNumCheck.cpp

182

My Clang-isms are spilling over. ;-) I've converted to the more verbose names.

clang-tidy/cert/CERTTidyModule.cpp

 #include "FloatLoopCounter.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
+#include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 

     // FIO
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "cert-fio38-c");
+    // ERR
+    CheckFactories.registerCheck<StrToNumCheck>(
+        "cert-err34-c");
   }
 };
 

clang-tidy/cert/CMakeLists.txt

   FloatLoopCounter.cpp
   SetLongJmpCheck.cpp
   StaticObjectExceptionCheck.cpp
+  StrToNumCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 
   LINK_LIBS
+  clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic

clang-tidy/cert/StrToNumCheck.h

+//===--- 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

clang-tidy/cert/StrToNumCheck.cpp

+//===--- 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

docs/clang-tidy/checks/cert-err34-c.rst

+.. 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>`_.

docs/clang-tidy/checks/list.rst

    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

    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

test/clang-tidy/cert-err34-c.c

+// 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
+}

test/clang-tidy/cert-err34-c.cpp

+// 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
+}