[clang-tidy] Rename android-file-open-flag and fix a bug

[yawanng]

Summary

1. Rename android-file-open-flag to android-cloexec-open.
2. Handle a case when the function is passed as an argument of a function-like macro.

[yawanng]

rename the check.

[chh]

Please update the subject line and summary.
The new diff shows only the renaming of check name and file names.
Were the other changes for format and macro lost or are they
going to be in another change later?

[yawanng]

Pull back the previous change. For some reason it's lost.

[chh]

I think you also need to change docs/clang-tidy/checks/list.rst.

[yawanng]

Change the file name in toctree.

clang-tidy/android/AndroidTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
-#include "FileOpenFlagCheck.h"
+#include "CloexecOpenCheck.h"
 
 using namespace clang::ast_matchers;
 

 class AndroidModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
-    CheckFactories.registerCheck<FileOpenFlagCheck>("android-file-open-flag");
+    CheckFactories.registerCheck<CloexecOpenCheck>("android-cloexec-open");
   }
 };
 

clang-tidy/android/CMakeLists.txt

 
 add_clang_library(clangTidyAndroidModule
   AndroidTidyModule.cpp
-  FileOpenFlagCheck.cpp
+  CloexecOpenCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tidy/android/CloexecOpenCheck.h

-//===--- FileOpenFlagCheck.h - clang-tidy----------------------------------===//
+//===--- CloexecOpenCheck.h - clang-tidy-----------------------------------===//
 //
 //                      The LLVM Compiler Infrastructure
 //

 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_FILE_OPEN_FLAG_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_FILE_OPEN_FLAG_H
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_OPEN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_OPEN_H
 
 #include "../ClangTidy.h"
 

 ///
 /// Only the symbolic 'O_CLOEXEC' macro definition is checked, not the concrete
 /// value.
-class FileOpenFlagCheck : public ClangTidyCheck {
+class CloexecOpenCheck : public ClangTidyCheck {
 public:
-  FileOpenFlagCheck(StringRef Name, ClangTidyContext *Context)
+  CloexecOpenCheck(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_ANDROID_FILE_OPEN_FLAG_H
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_OPEN_H

clang-tidy/android/CloexecOpenCheck.cpp

-//===--- FileOpenFlagCheck.cpp - clang-tidy--------------------------------===//
+//===--- CloexecOpenCheck.cpp - clang-tidy---------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //

 //
 //===----------------------------------------------------------------------===//
 
-#include "FileOpenFlagCheck.h"
+#include "CloexecOpenCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"

 namespace {
 static constexpr const char *O_CLOEXEC = "O_CLOEXEC";
 
-bool HasCloseOnExecFlag(const Expr *Flags, const SourceManager &SM,
+bool hasCloseOnExecFlag(const Expr *Flags, const SourceManager &SM,
                         const LangOptions &LangOpts) {
   // If the Flag is an integer constant, check it.
   if (isa<IntegerLiteral>(Flags)) {
-    if (!SM.isMacroBodyExpansion(Flags->getLocStart()))
+    if (!SM.isMacroBodyExpansion(Flags->getLocStart()) &&
+        !SM.isMacroArgExpansion(Flags->getLocStart()))
       return false;
 
     // Get the Marco name.

   // If it's a binary OR operation.
   if (const auto *BO = dyn_cast<BinaryOperator>(Flags))
     if (BO->getOpcode() == clang::BinaryOperatorKind::BO_Or)
-      return HasCloseOnExecFlag(BO->getLHS()->IgnoreParenCasts(), SM,
+      return hasCloseOnExecFlag(BO->getLHS()->IgnoreParenCasts(), SM,
                                 LangOpts) ||
-             HasCloseOnExecFlag(BO->getRHS()->IgnoreParenCasts(), SM, LangOpts);
+             hasCloseOnExecFlag(BO->getRHS()->IgnoreParenCasts(), SM, LangOpts);
 
   // Otherwise, assume it has the flag.
   return true;
 }
 } // namespace
 
-void FileOpenFlagCheck::registerMatchers(MatchFinder *Finder) {
+void CloexecOpenCheck::registerMatchers(MatchFinder *Finder) {
   auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter())));
 
   Finder->addMatcher(

       this);
 }
 
-void FileOpenFlagCheck::check(const MatchFinder::MatchResult &Result) {
+void CloexecOpenCheck::check(const MatchFinder::MatchResult &Result) {
   const Expr *FlagArg = nullptr;
   if (const auto *OpenFnCall = Result.Nodes.getNodeAs<CallExpr>("openFn"))
     FlagArg = OpenFnCall->getArg(1);

 
   // Check the required flag.
   SourceManager &SM = *Result.SourceManager;
-  if (HasCloseOnExecFlag(FlagArg->IgnoreParenCasts(), SM,
+  if (hasCloseOnExecFlag(FlagArg->IgnoreParenCasts(), SM,
                          Result.Context->getLangOpts()))
     return;
 
-  SourceLocation EndLoc = Lexer::getLocForEndOfToken(
-      FlagArg->getLocEnd(), 0, SM, Result.Context->getLangOpts());
+  SourceLocation EndLoc =
+      Lexer::getLocForEndOfToken(SM.getFileLoc(FlagArg->getLocEnd()), 0, SM,
+                                 Result.Context->getLangOpts());
 
   diag(EndLoc, "%0 should use %1 where possible")
       << FD << O_CLOEXEC

docs/ReleaseNotes.rst

 Improvements to clang-tidy
 --------------------------
 
-- New `android-file-open-flag
-  <http://clang.llvm.org/extra/clang-tidy/checks/android-file-open-flag.html>`_ check
+- New `android-cloexec-open
+  <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-open.html>`_ check
 
   Checks if the required file flag ``O_CLOEXEC`` exists in ``open()``,
   ``open64()`` and ``openat()``.

docs/clang-tidy/checks/android-cloexec-open.rst

-.. title:: clang-tidy - android-file-open-flag
+.. title:: clang-tidy - android-cloexec-open
 
-android-file-open-flag
-======================
+android-cloexec-open
+====================
 
 A common source of security bugs is code that opens a file without using the
 ``O_CLOEXEC`` flag.  Without that flag, an opened sensitive file would remain

docs/clang-tidy/checks/list.rst

 =================
 
 .. toctree::
-   android-file-open-flag
+   android-cloexec-open
    boost-use-to-string
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp

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

+// RUN: %check_clang_tidy %s android-cloexec-open %t
+
+#define O_RDWR 1
+#define O_EXCL 2
+#define __O_CLOEXEC 3
+#define O_CLOEXEC __O_CLOEXEC
+#define TEMP_FAILURE_RETRY(exp) \
+  ({                            \
+    int _rc;                    \
+    do {                        \
+      _rc = (exp);              \
+    } while (_rc == -1);        \
+  })
+
+extern "C" int open(const char *fn, int flags, ...);
+extern "C" int open64(const char *fn, int flags, ...);
+extern "C" int openat(int dirfd, const char *pathname, int flags, ...);
+
+void a() {
+  open("filename", O_RDWR);
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'open' should use O_CLOEXEC where possible [android-cloexec-open]
+  // CHECK-FIXES: O_RDWR | O_CLOEXEC
+  TEMP_FAILURE_RETRY(open("filename", O_RDWR));
+  // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: 'open' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_CLOEXEC
+  open("filename", O_RDWR | O_EXCL);
+  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: 'open' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
+  TEMP_FAILURE_RETRY(open("filename", O_RDWR | O_EXCL));
+  // CHECK-MESSAGES: :[[@LINE-1]]:54: warning: 'open' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
+}
+
+void b() {
+  open64("filename", O_RDWR);
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 'open64' should use O_CLOEXEC where possible [android-cloexec-open]
+  // CHECK-FIXES: O_RDWR | O_CLOEXEC
+  TEMP_FAILURE_RETRY(open64("filename", O_RDWR));
+  // CHECK-MESSAGES: :[[@LINE-1]]:47: warning: 'open64' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_CLOEXEC
+  open64("filename", O_RDWR | O_EXCL);
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'open64' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
+  TEMP_FAILURE_RETRY(open64("filename", O_RDWR | O_EXCL));
+  // CHECK-MESSAGES: :[[@LINE-1]]:56: warning: 'open64' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
+}
+
+void c() {
+  openat(0, "filename", O_RDWR);
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 'openat' should use O_CLOEXEC where possible [android-cloexec-open]
+  // CHECK-FIXES: O_RDWR | O_CLOEXEC
+  TEMP_FAILURE_RETRY(openat(0, "filename", O_RDWR));
+  // CHECK-MESSAGES: :[[@LINE-1]]:50: warning: 'openat' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_CLOEXEC
+  openat(0, "filename", O_RDWR | O_EXCL);
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: 'openat' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
+  TEMP_FAILURE_RETRY(openat(0, "filename", O_RDWR | O_EXCL));
+  // CHECK-MESSAGES: :[[@LINE-1]]:59: warning: 'openat' should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
+}
+
+void f() {
+  open("filename", 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'open' should use O_CLOEXEC where possible [android-cloexec-open]
+  // CHECK-FIXES: 3 | O_CLOEXEC
+  TEMP_FAILURE_RETRY(open("filename", 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: 'open' should use O_CLOEXEC where
+  // CHECK-FIXES: 3 | O_CLOEXEC
+  open64("filename", 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'open64' should use O_CLOEXEC where possible [android-cloexec-open]
+  // CHECK-FIXES: 3 | O_CLOEXEC
+  TEMP_FAILURE_RETRY(open64("filename", 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: 'open64' should use O_CLOEXEC where
+  // CHECK-FIXES: 3 | O_CLOEXEC
+  openat(0, "filename", 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'openat' should use O_CLOEXEC where possible [android-cloexec-open]
+  // CHECK-FIXES: 3 | O_CLOEXEC
+  TEMP_FAILURE_RETRY(openat(0, "filename", 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: 'openat' should use O_CLOEXEC where
+  // CHECK-FIXES: 3 | O_CLOEXEC
+
+  int flag = 3;
+  open("filename", flag);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open("filename", flag));
+  // CHECK-MESSAGES-NOT: warning:
+  open64("filename", flag);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open64("filename", flag));
+  // CHECK-MESSAGES-NOT: warning:
+  openat(0, "filename", flag);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(openat(0, "filename", flag));
+  // CHECK-MESSAGES-NOT: warning:
+}
+
+namespace i {
+int open(const char *pathname, int flags, ...);
+int open64(const char *pathname, int flags, ...);
+int openat(int dirfd, const char *pathname, int flags, ...);
+
+void d() {
+  open("filename", O_RDWR);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open("filename", O_RDWR));
+  // CHECK-MESSAGES-NOT: warning:
+  open64("filename", O_RDWR);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open64("filename", O_RDWR));
+  // CHECK-MESSAGES-NOT: warning:
+  openat(0, "filename", O_RDWR);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(openat(0, "filename", O_RDWR));
+  // CHECK-MESSAGES-NOT: warning:
+}
+
+} // namespace i
+
+void e() {
+  open("filename", O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open("filename", O_CLOEXEC));
+  // CHECK-MESSAGES-NOT: warning:
+  open("filename", O_RDWR | O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open("filename", O_RDWR | O_CLOEXEC));
+  // CHECK-MESSAGES-NOT: warning:
+  open("filename", O_RDWR | O_CLOEXEC | O_EXCL);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open("filename", O_RDWR | O_CLOEXEC | O_EXCL));
+  // CHECK-MESSAGES-NOT: warning:
+  open64("filename", O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open64("filename", O_CLOEXEC));
+  // CHECK-MESSAGES-NOT: warning:
+  open64("filename", O_RDWR | O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open64("filename", O_RDWR | O_CLOEXEC));
+  // CHECK-MESSAGES-NOT: warning:
+  open64("filename", O_RDWR | O_CLOEXEC | O_EXCL);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(open64("filename", O_RDWR | O_CLOEXEC | O_EXCL));
+  // CHECK-MESSAGES-NOT: warning:
+  openat(0, "filename", O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(openat(0, "filename", O_CLOEXEC));
+  // CHECK-MESSAGES-NOT: warning:
+  openat(0, "filename", O_RDWR | O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(openat(0, "filename", O_RDWR | O_CLOEXEC));
+  // CHECK-MESSAGES-NOT: warning:
+  openat(0, "filename", O_RDWR | O_CLOEXEC | O_EXCL);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(openat(0, "filename", O_RDWR | O_CLOEXEC | O_EXCL));
+  // CHECK-MESSAGES-NOT: warning:
+}
+
+class G {
+public:
+  int open(const char *pathname, int flags, ...);
+  int open64(const char *pathname, int flags, ...);
+  int openat(int dirfd, const char *pathname, int flags, ...);
+
+  void h() {
+    open("filename", O_RDWR);
+    // CHECK-MESSAGES-NOT: warning:
+    TEMP_FAILURE_RETRY(open("filename", O_RDWR));
+    // CHECK-MESSAGES-NOT: warning:
+    open64("filename", O_RDWR);
+    // CHECK-MESSAGES-NOT: warning:
+    TEMP_FAILURE_RETRY(open64("filename", O_RDWR));
+    // CHECK-MESSAGES-NOT: warning:
+    openat(0, "filename", O_RDWR);
+    // CHECK-MESSAGES-NOT: warning:
+    TEMP_FAILURE_RETRY(openat(0, "filename", O_RDWR));
+    // CHECK-MESSAGES-NOT: warning:
+  }
+};

test/clang-tidy/android-file-open-flag.cpp

-// RUN: %check_clang_tidy %s android-file-open-flag %t
-
-#define O_RDWR 1
-#define O_EXCL 2
-#define __O_CLOEXEC 3
-#define O_CLOEXEC __O_CLOEXEC
-
-extern "C" int open(const char *fn, int flags, ...);
-extern "C" int open64(const char *fn, int flags, ...);
-extern "C" int openat(int dirfd, const char *pathname, int flags, ...);
-
-void a() {
-  open("filename", O_RDWR);
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag]
-  // CHECK-FIXES: O_RDWR | O_CLOEXEC
-  open("filename", O_RDWR | O_EXCL);
-  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: 'open' should use O_CLOEXEC where
-  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
-}
-
-void b() {
-  open64("filename", O_RDWR);
-  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag]
-  // CHECK-FIXES: O_RDWR | O_CLOEXEC
-  open64("filename", O_RDWR | O_EXCL);
-  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'open64' should use O_CLOEXEC where
-  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
-}
-
-void c() {
-  openat(0, "filename", O_RDWR);
-  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag]
-  // CHECK-FIXES: O_RDWR | O_CLOEXEC
-  openat(0, "filename", O_RDWR | O_EXCL);
-  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: 'openat' should use O_CLOEXEC where
-  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
-}
-
-void f() {
-  open("filename", 3);
-  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag]
-  // CHECK-FIXES: 3 | O_CLOEXEC
-  open64("filename", 3);
-  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag]
-  // CHECK-FIXES: 3 | O_CLOEXEC
-  openat(0, "filename", 3);
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag]
-  // CHECK-FIXES: 3 | O_CLOEXEC
-
-  int flag = 3;
-  open("filename", flag);
-  // CHECK-MESSAGES-NOT: warning:
-  open64("filename", flag);
-  // CHECK-MESSAGES-NOT: warning:
-  openat(0, "filename", flag);
-  // CHECK-MESSAGES-NOT: warning:
-}
-
-namespace i {
-int open(const char *pathname, int flags, ...);
-int open64(const char *pathname, int flags, ...);
-int openat(int dirfd, const char *pathname, int flags, ...);
-
-void d() {
-  open("filename", O_RDWR);
-  // CHECK-MESSAGES-NOT: warning:
-  open64("filename", O_RDWR);
-  // CHECK-MESSAGES-NOT: warning:
-  openat(0, "filename", O_RDWR);
-  // CHECK-MESSAGES-NOT: warning:
-}
-
-} // namespace i
-
-void e() {
-  open("filename", O_CLOEXEC);
-  // CHECK-MESSAGES-NOT: warning:
-  open("filename", O_RDWR | O_CLOEXEC);
-  // CHECK-MESSAGES-NOT: warning:
-  open("filename", O_RDWR | O_CLOEXEC | O_EXCL);
-  // CHECK-MESSAGES-NOT: warning:
-  open64("filename", O_CLOEXEC);
-  // CHECK-MESSAGES-NOT: warning:
-  open64("filename", O_RDWR | O_CLOEXEC);
-  // CHECK-MESSAGES-NOT: warning:
-  open64("filename", O_RDWR | O_CLOEXEC | O_EXCL);
-  // CHECK-MESSAGES-NOT: warning:
-  openat(0, "filename", O_CLOEXEC);
-  // CHECK-MESSAGES-NOT: warning:
-  openat(0, "filename", O_RDWR | O_CLOEXEC);
-  // CHECK-MESSAGES-NOT: warning:
-  openat(0, "filename", O_RDWR | O_CLOEXEC | O_EXCL);
-  // CHECK-MESSAGES-NOT: warning:
-}
-
-class G {
-public:
-  int open(const char *pathname, int flags, ...);
-  int open64(const char *pathname, int flags, ...);
-  int openat(int dirfd, const char *pathname, int flags, ...);
-
-  void h() {
-    open("filename", O_RDWR);
-    // CHECK-MESSAGES-NOT: warning:
-    open64("filename", O_RDWR);
-    // CHECK-MESSAGES-NOT: warning:
-    openat(0, "filename", O_RDWR);
-    // CHECK-MESSAGES-NOT: warning:
-  }
-};