[PATCH] Add checker discouraging definition of variadic function definitions in C++

[aaron.ballman]

Summary

C-style variadic functions (using an ellipsis) can be dangerous in C++ due to the inherit lack of type safety with argument passing. Better alternatives exist, such as function currying (like STL stream objects use), or function parameter packs. This patch adds a checker to diagnose definitions of variadic functions in C++ code, but still allows variadic function declarations, as those can be safely used to good effect for SFINAE patterns.

This patch corresponds to the CERT C++ Coding Standard rule: https://www.securecoding.cert.org/confluence/display/cplusplus/DCL50-CPP.+Do+not+define+a+C-style+variadic+function

[sbenza]

See comment regarding //CHECKs

test/clang-tidy/cert-variadic-function-def.cpp

4

All other tests I've read/written put the //CHECK after the matched lines.
We should be consistent with that.

[aaron.ballman]

Thank you for the note about checks -- I hadn't noticed that. I've fixed and committed in r249343.

Thank you for the quick review!

~Aaron

[• dberlin]

docs/clang-tidy/checks/cert-variadic-function-def.rst

13

I'm sure this is oversight on CERT's part, but their website actually has terms of use (click the terms of use at the bottom of the page) that says this can't be copied/reused, and here you are, copying it.
It explicit says: "
Use of the Service. You may only display the content of the Service for your own personal use (i.e., non-commercial use) and may not otherwise copy, reproduce, alter, modify, create derivative works, or publicly display any content. "

Before this is accepted, someone should email cert and say "hey, uh, yeah, this seems bad", and get them to okay you doing this.
I'm sure they'll go and fix this.

clang-tidy/cert/CERTTidyModule.cpp

 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
+#include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {

   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     // C++ checkers
     // DCL
+    CheckFactories.registerCheck<VariadicFunctionDefCheck>(
+        "cert-dcl50-cpp");
     CheckFactories.registerCheck<misc::NewDeleteOverloadsCheck>(
         "cert-dcl54-cpp");
     CheckFactories.registerCheck<google::build::UnnamedNamespaceInHeaderCheck>(

clang-tidy/cert/CMakeLists.txt

 
 add_clang_library(clangTidyCERTModule
   CERTTidyModule.cpp
+  VariadicFunctionDefCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tidy/cert/VariadicFunctionDefCheck.h

+//===--- VariadicFunctionDefCheck.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_VARIADICFUNCTIONDEF_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_VARIADICFUNCTIONDEF_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Guards against any C-style variadic function definitions (not declarations).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-variadic-function-def.html
+class VariadicFunctionDefCheck : public ClangTidyCheck {
+public:
+  VariadicFunctionDefCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_VARIADICFUNCTIONDEF_H
+

clang-tidy/cert/VariadicFunctionDefCheck.cpp

+//===--- VariadicfunctiondefCheck.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 "VariadicFunctionDefCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void VariadicFunctionDefCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // We only care about function *definitions* that are variadic.
+  Finder->addMatcher(functionDecl(isDefinition(), isVariadic()).bind("func"),
+                     this);
+}
+
+void VariadicFunctionDefCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("func");
+
+  diag(FD->getLocation(),
+       "do not define a C-style variadic function; consider using a function "
+       "parameter pack or currying instead");
+}
+
+} // namespace tidy
+} // namespace clang
+

docs/clang-tidy/checks/cert-variadic-function-def.rst

+cert-dcl50-cpp
+========================
+
+A variadic function using an ellipsis has no mechanisms to check the type safety
+of arguments being passed to the function or to check that the number of
+arguments being passed matches the semantics of the function definition.
+Consequently, a runtime call to a C-style variadic function that passes
+inappropriate arguments yields undefined behavior. Such undefined behavior could
+be exploited to run arbitrary code.
+
+This check corresponds to the CERT C++ Coding Standard rule
+`DCL50-CPP. Do not define a C-style variadic function
+<https://www.securecoding.cert.org/confluence/display/cplusplus/DCL50-CPP.+Do+not+define+a+C-style+variadic+function>`_.

docs/clang-tidy/checks/list.rst

 =========================
 
 .. toctree::
+   cert-variadic-function-def
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace

test/clang-tidy/cert-variadic-function-def.cpp

+// RUN: %python %S/check_clang_tidy.py %s cert-dcl50-cpp %t
+
+// Variadic function definitions are diagnosed.
+// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: do not define a C-style variadic function; consider using a function parameter pack or currying instead [cert-dcl50-cpp]
+void f1(int, ...) {}
+
+// Variadic function *declarations* are not diagnosed.
+void f2(int, ...); // ok
+
+// Function parameter packs are good, however.
+template <typename Arg, typename... Ts>
+void f3(Arg F, Ts... Rest) {}
+
+struct S {
+  void f(int, ...); // ok
+  // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: do not define a C-style variadic function; consider using a function parameter pack or currying instead [cert-dcl50-cpp]
+  void f1(int, ...) {}
+};