[clang-tidy] Add a close-on-exec check on dup() in Android module.

[yawanng]

Summary

dup() is better to be replaced by fcntl() to avoid file descriptor leakage.

[alexfh]

See the comment on D35372.

[hokein]

clang-tidy/android/CloexecCheck.h

91 ↗

(On Diff #110654)

This method seems only be used in one check. Maybe move it to the implementation of that specific check? And you can make the private members FuncBindingStr and FuncDeclBindingStr to protected member.

test/clang-tidy/android-cloexec-dup.cpp

7 ↗

(On Diff #110654)

nit: trailing ;, the same below. Please apply this to other patches as well.

[yawanng]

clang-tidy/android/CloexecCheck.h

91 ↗

(On Diff #110654)

Actually, this method will be used in three checks.

[hokein]

LGTM

clang-tidy/android/CloexecCheck.h

91 ↗

(On Diff #110654)

Ah, ok. Sorry for missing it.

clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp

 //===--- AndroidTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "CloexecCreatCheck.h"
+#include "CloexecDupCheck.h"
 #include "CloexecFopenCheck.h"
 #include "CloexecMemfdCreateCheck.h"
 #include "CloexecOpenCheck.h"
 #include "CloexecSocketCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace android {
 
 /// This module is for Android specific checks.
 class AndroidModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<CloexecCreatCheck>("android-cloexec-creat");
+    CheckFactories.registerCheck<CloexecDupCheck>("android-cloexec-dup");
     CheckFactories.registerCheck<CloexecFopenCheck>("android-cloexec-fopen");
     CheckFactories.registerCheck<CloexecMemfdCreateCheck>(
         "android-cloexec-memfd-create");
     CheckFactories.registerCheck<CloexecOpenCheck>("android-cloexec-open");
     CheckFactories.registerCheck<CloexecSocketCheck>("android-cloexec-socket");
   }
 };
 

clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAndroidModule
   AndroidTidyModule.cpp
   CloexecCheck.cpp
   CloexecCreatCheck.cpp
+  CloexecDupCheck.cpp
   CloexecFopenCheck.cpp
   CloexecMemfdCreateCheck.cpp
   CloexecOpenCheck.cpp
   CloexecSocketCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/trunk/clang-tidy/android/CloexecCheck.h

protected:
   ///   fopen(in_file, "re");
   /// \endcode
   ///
   /// \param [out] Result MatchResult from AST matcher.
   /// \param Mode The required mode char.
   /// \param ArgPos The 0-based position of the flag argument.
   void insertStringFlag(const ast_matchers::MatchFinder::MatchResult &Result,
                         const char Mode, const int ArgPos);
+
+  /// Helper function to get the spelling of a particular argument.
+  StringRef getSpellingArg(const ast_matchers::MatchFinder::MatchResult &Result,
+                           int N) const;
 };
 
 } // namespace android
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_H

clang-tools-extra/trunk/clang-tidy/android/CloexecCheck.cpp

const std::string &ReplacementText = buildFixMsgForStringFlag(
       ModeArg, *Result.SourceManager, Result.Context->getLangOpts(), Mode);
 
   diag(ModeArg->getLocStart(), "use %0 mode '%1' to set O_CLOEXEC")
       << FD << std::string(1, Mode)
       << FixItHint::CreateReplacement(ModeArg->getSourceRange(),
                                       ReplacementText);
 }
 
+StringRef CloexecCheck::getSpellingArg(const MatchFinder::MatchResult &Result,
+                                       int N) const {
+  const auto *MatchedCall = Result.Nodes.getNodeAs<CallExpr>(FuncBindingStr);
+  const SourceManager &SM = *Result.SourceManager;
+  return Lexer::getSourceText(
+      CharSourceRange::getTokenRange(MatchedCall->getArg(N)->getSourceRange()),
+      SM, Result.Context->getLangOpts());
+}
+
 } // namespace android
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/android/CloexecDupCheck.h

+//===--- CloexecDupCheck.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_ANDROID_CLOEXEC_DUP_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_DUP_H
+
+#include "CloexecCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+/// dup() is better to be replaced by fcntl(), which has close-on-exec flag.
+/// Find the usage of dup() and redirect user to use fcntl().
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-dup.html
+class CloexecDupCheck : public CloexecCheck {
+public:
+  CloexecDupCheck(StringRef Name, ClangTidyContext *Context)
+      : CloexecCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace android
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_DUP_H

clang-tools-extra/trunk/clang-tidy/android/CloexecDupCheck.cpp

+//===--- CloexecDupCheck.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 "CloexecDupCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+void CloexecDupCheck::registerMatchers(MatchFinder *Finder) {
+  registerMatchersImpl(Finder,
+                       functionDecl(returns(isInteger()), hasName("dup"),
+                                    hasParameter(0, hasType(isInteger()))));
+}
+
+void CloexecDupCheck::check(const MatchFinder::MatchResult &Result) {
+  const std::string &ReplacementText =
+      (Twine("fcntl(") + getSpellingArg(Result, 0) + ", F_DUPFD_CLOEXEC)")
+          .str();
+
+  replaceFunc(Result,
+              "prefer fcntl() to dup() because fcntl() allows F_DUPFD_CLOEXEC",
+              ReplacementText);
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

* **performance-implicit-cast-in-loop** was renamed to
       <http://clang.llvm.org/extra/clang-tidy/checks/performance-implicit-conversion-in-loop.html>`_
     * **readability-implicit-bool-cast** was renamed to
       `readability-implicit-bool-conversion
       <http://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html>`_;
       the check's options were renamed as follows:
       ``AllowConditionalIntegerCasts`` -> ``AllowIntegerConditions``,
       ``AllowConditionalPointerCasts`` -> ``AllowPointerConditions``.
 
+- New `android-cloexec-dup
+  <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-dup.html>`_ check
+
+  Detects usage of ``dup()``.
+
 - New `android-cloexec-memfd_create
   <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-memfd_create.html>`_ check
 
   Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument
   of ``memfd_create()``.
 
 - New `bugprone-integer-division
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-integer-division.html>`_ check

clang-tools-extra/trunk/docs/clang-tidy/checks/android-cloexec-dup.rst

+.. title:: clang-tidy - android-cloexec-dup
+
+android-cloexec-dup
+===================
+
+The usage of ``dup()`` is not recommended, it's better to use ``fcntl()``,
+which can set the close-on-exec flag. Otherwise, an opened sensitive file would
+remain open across a fork+exec to a lower-privileged SELinux domain.
+
+Examples:
+
+.. code-block:: c++
+
+  int fd = dup(oldfd);
+
+  // becomes
+
+  int fd = fcntl(oldfd, F_DUPFD_CLOEXEC);

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    android-cloexec-creat
+   android-cloexec-dup
    android-cloexec-fopen
    android-cloexec-memfd-create
    android-cloexec-open
    android-cloexec-socket
    boost-use-to-string
    bugprone-integer-division
    bugprone-suspicious-memset-usage
    bugprone-undefined-memory-manipulation

clang-tools-extra/trunk/test/clang-tidy/android-cloexec-dup.cpp

+// RUN: %check_clang_tidy %s android-cloexec-dup %t
+
+extern "C" int dup(int oldfd);
+void f() {
+  dup(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer fcntl() to dup() because fcntl() allows F_DUPFD_CLOEXEC [android-cloexec-dup]
+  // CHECK-FIXES: fcntl(1, F_DUPFD_CLOEXEC);
+  int oldfd = 0;
+  dup(oldfd);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer
+  // CHECK-FIXES: fcntl(oldfd, F_DUPFD_CLOEXEC);
+}
+
+namespace i {
+int dup(int oldfd);
+void g() {
+  dup(0);
+  int oldfd = 1;
+  dup(oldfd);
+}
+} // namespace i
+
+class C {
+public:
+  int dup(int oldfd);
+  void h() {
+    dup(0);
+    int oldfd = 1;
+    dup(oldfd);
+  }
+};