[clang-tidy][Part1] Add a new module Android and three new checks.

[yawanng]

Summary

A common source of security bugs is code that opens a file descriptors without using the O_CLOEXEC flag. (Without that flag, an opened sensitive file would remain open across a fork+exec to a lower-privileged SELinux domain, leaking that sensitive data.).

Add a new Android module and one checks in clang-tidy.

• open(), openat(), and open64() should include O_CLOEXEC in their flags argument. [android-file-open-flag]

Links to part2 and part3:
https://reviews.llvm.org/D33745
https://reviews.llvm.org/D33747

[yawanng]

Add unit test file.

[Eugene.Zelenko]

Please run clang-format over your code.

Please add documentation and mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[srhines]

clang-tidy/android/FileDescriptorCheck.cpp

65 ↗

(On Diff #99374)

Use "%0 " instead of the 3 function names, so that you can use << to print the name as part of the diagnostic. You can get the name by binding the FunctionDecl in the matcher you create (and then that lets you do "FD->getName()"). You can look at something like FasterStringFindCheck.cpp for a good example of this (search for "%0").

76 ↗

(On Diff #99374)

Using O_CLOEXEC here is potentially a problem, since most of our compiles are cross-compiles. If O_CLOEXEC is different on the target platform than the host platform (where this code is being compiled), this check would fail. Perhaps we can get the value of O_CLOEXEC as defined for the translation unit (and if it isn't defined, that's already a pretty big indicator that any use of these functions will be wrong). Alternately, maybe just re-parsing for "O_CLOEXEC" is better.

[joerg]

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

[srhines]

In D33304#758621, @joerg wrote:

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

[yawanng]

clang-tidy/android/FileDescriptorCheck.cpp

76 ↗

(On Diff #99374)

I also think re-parsing is better. I will think about this and update soon.

[aaron.ballman]

In D33304#758624, @srhines wrote:

In D33304#758621, @joerg wrote:

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C. I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

clang-tidy/android/FileDescriptorCheck.cpp

66 ↗

(On Diff #99447)

This can just pass FD instead of FD->getName() -- that will also properly quote the function call.

This diagnostic doesn't tell the user what's wrong with their code, just how to silence the warning. I'm not keen on "must" in this diagnostic either. We don't usually tell the user they must do something unless they truly must. We usually waffle a bit and use phrase like "<reasons why the code is bad>; consider using O_CLOEXEC instead".

[alexfh]

In D33304#758713, @aaron.ballman wrote:

In D33304#758624, @srhines wrote:

In D33304#758621, @joerg wrote:

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C.

I think AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT?

I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

I'm strongly against bloating "misc" module. It's more or less the last resort, a place for checks we have found no better place for. The proposed "bugprone" module is an attempt to address this by pulling out a large part of "misc" to a place with more definite name and purpose. However, in the case of this check we seem to have enough good (and more specific) alternatives to default to "misc" or even "bugprone".

[Eugene.Zelenko]

clang-tidy/android/AndroidTidyModule.cpp

16

Are readability headers are really necessary?

[aaron.ballman]

In D33304#758808, @alexfh wrote:

In D33304#758713, @aaron.ballman wrote:

In D33304#758624, @srhines wrote:

In D33304#758621, @joerg wrote:

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C.

I think AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT?

If there are plans to add more checks, then yes. However, I think I'd prefer to see at least 2-3 checks in the work (or have some commitment for doing at least that many checks) before we add a module for it. I mostly worry about adding a single check and then nothing else. (No matter what module name we're talking about, btw.) I'd be fine with android, posix, or unix, depending on the nature of the checks.

I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

I'm strongly against bloating "misc" module. It's more or less the last resort, a place for checks we have found no better place for. The proposed "bugprone" module is an attempt to address this by pulling out a large part of "misc" to a place with more definite name and purpose. However, in the case of this check we seem to have enough good (and more specific) alternatives to default to "misc" or even "bugprone".

I was hesitant to suggest misc, but I was hoping to avoid adding a module with a single check under it and no commitment for further ones.

[yawanng]

In D33304#758871, @aaron.ballman wrote:

In D33304#758808, @alexfh wrote:

In D33304#758713, @aaron.ballman wrote:

In D33304#758624, @srhines wrote:

In D33304#758621, @joerg wrote:

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C.

I think AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT?

If there are plans to add more checks, then yes. However, I think I'd prefer to see at least 2-3 checks in the work (or have some commitment for doing at least that many checks) before we add a module for it. I mostly worry about adding a single check and then nothing else. (No matter what module name we're talking about, btw.) I'd be fine with android, posix, or unix, depending on the nature of the checks.

I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

I'm strongly against bloating "misc" module. It's more or less the last resort, a place for checks we have found no better place for. The proposed "bugprone" module is an attempt to address this by pulling out a large part of "misc" to a place with more definite name and purpose. However, in the case of this check we seem to have enough good (and more specific) alternatives to default to "misc" or even "bugprone".

I was hesitant to suggest misc, but I was hoping to avoid adding a module with a single check under it and no commitment for further ones.

There are also two other requirements(not yet implemented). There will be more checks following up.

[alexfh]

clang-tidy/android/FileDescriptorCheck.cpp

25 ↗

(On Diff #99447)

functionDecl() is implicitly allOf.

26 ↗

(On Diff #99447)

isExpansionInFileMatching(...) is a dependency on the implementation of the library. The fact that #include <fcntl.h> provides this macro doesn't mean the macro is defined in this header (it could be in another transitively included header). Same below.

26 ↗

(On Diff #99447)

asString("int") is wasteful, since it needs to allocate the string and print the return type name there. hasType(isInteger()) is a much better alternative.

28 ↗

(On Diff #99447)

matchesName is wasteful, if you need just to compare the name. You probably want to use hasAnyName("open", "open64").

90 ↗

(On Diff #99447)

http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

test/clang-tidy/android-file-descriptor.cpp

3 ↗

(On Diff #99447)

We don't use actual library headers in the tests. Instead, please add mock declarations of the API you need (open, open64, O_ flags, etc.).

[yawanng]

I will make some major changes to this CL based on the current suggestions from reviewers and update it for further review later. Thank you for the valuable advice.

[Eugene.Zelenko]

It's also necessary to mention new checks group in docs/clang-tidy/index.rst.

[yawanng]

Modify the detection algorithm. Add another two checks and corresponding tests as well as the docs.

[yawanng]

Hi, I have updated this CL for review. Thank you very much :)

[Eugene.Zelenko]

clang-tidy/utils/ExprToStr.cpp

23 ↗

(On Diff #100777)

Please add empty line.

clang-tidy/utils/ExprToStr.h

22 ↗

(On Diff #100777)

Please add empty line.

docs/ReleaseNotes.rst

69

May be detect usage of `creat()` will be better?

docs/clang-tidy/checks/android-creat-usage.rst

5 ↗

(On Diff #100777)

Please enclose creat() and open() in `` and add empty line before.

docs/clang-tidy/checks/android-file-open-flag.rst

6

Please add empty line.

11

Unnecessary empty line.

docs/clang-tidy/checks/android-fopen-mode.rst

5 ↗

(On Diff #100777)

Please add empty line.

[alexfh]

It's awesome to see another two checks, and it seems to justify adding a separate module, but I'd prefer the other two checks to be sent for review as separate patches to make the review easier and faster.

[yawanng]

In D33304#768731, @alexfh wrote:

It's awesome to see another two checks, and it seems to justify adding a separate module, but I'd prefer the other two checks to be sent for review as separate patches to make the review easier and faster.

The other two checks are relatively small and simple. I understand this may make the review process longer and we could wait for that. Please take your time :-) Thank you!

[alexfh]

In D33304#768961, @yawanng wrote:

In D33304#768731, @alexfh wrote:

It's awesome to see another two checks, and it seems to justify adding a separate module, but I'd prefer the other two checks to be sent for review as separate patches to make the review easier and faster.

The other two checks are relatively small and simple. I understand this may make the review process longer and we could wait for that. Please take your time :-) Thank you!

It's not only longer to review larger patches, it's also less convenient for the reviewers. Please send each check as a separate patch (the addition of the module can be in one of them). Phabricator has decent support for dependent patches, so it shouldn't add much work on your side.

[yawanng]

Split the commit to three ones that each of them contains one. This is the first part.

[hokein]

Thanks for the contributions.

All your three checks are not android specific -- because they are checking POSIX APIs (open, creat, fopen), which are more likely related to POSIX. So personally, I'm +1 on a "posix" module, instead of "android", but wait to see other reviewers' opinions before renaming it.

[alexfh]

In D33304#771215, @hokein wrote:

Thanks for the contributions.

All your three checks are not android specific -- because they are checking POSIX APIs (open, creat, fopen), which are more likely related to POSIX. So personally, I'm +1 on a "posix" module, instead of "android", but wait to see other reviewers' opinions before renaming it.

IIUC, these checks enforce a certain - Android-specific - way of using POSIX APIs. I'm not sure if the recommendations are universally useful. Or am I mistaken?

[yawanng]

Format changes.

[hokein]

In D33304#771493, @alexfh wrote:

IIUC, these checks enforce a certain - Android-specific - way of using POSIX APIs. I'm not sure if the recommendations are universally useful. Or am I mistaken?

OK, that makes sense. I may miss some background context.

docs/clang-tidy/index.rst

58

Add some words explaining this module?

[alexfh]

clang-tidy/android/FileOpenFlagCheck.cpp

61

No need for this variable, since it's just used once. Same below.

63–66

No need to replace the text with itself. Just insert the part that is missing. The more local the changes - the fewer are chances of conflicts.

clang-tidy/android/FileOpenFlagCheck.h

39

This doesn't have to be a class member. It could be local to the function where it's used or to the .cc file.

docs/clang-tidy/checks/android-file-open-flag.rst

7

I'm not sure about using of "has been" here. Maybe just use present tense? You might want to consult a nearby native speaker though. Alternatively, you might want to rephrase this as "Opening files using POSIX `open or similar system calls without specifying the O_CLOEXEC` flag is a common source of security bugs."

10

Missing period before "Functions".

10

Just "functions" is too broad. I'd say "open-like functions", "POSIX functions that open files" or something like that.

[yawanng]

docs/clang-tidy/checks/android-file-open-flag.rst

7

I copied these sentences from the requests written by a native speaker, I think. But after talking with another native speaker, yes, present tense seems to be better here. Thanks :)

[hokein]

clang-tidy/android/FileOpenFlagCheck.cpp

29

I'd put the hasAnyName matcher in front of hasParameter which follows the order of function declaration.

The same below.

47

How about

const Expr *FlagArg = nullptr;
if (const auto* OpenFnCall = Result.Nodes.getXXX()) {
  // ...
} else if (const auto* OpenAtFnCall = Result.Nodes.getXX()) {
  // ...
}
assert(FlagArg);

?

62

Instead of using getLangOpts(), you should use Result.Context.getLangOpts().

68

No need to be a class member method, put it inside this translation unit.

80

You could use Lexer::getImmediateMacroName(Loc, SM, Opts); to get the marco name, or doesn't it work here?

85

I think you can use early return here. With that you don't need to maintain the flag variable IsFlagIn, so the code structure like:

if (isa<..>) {
   
    return ...;
}

if (isa<BinaryOperator>()) {
    // ...
    return ...;
}
retrun false;

89

You can use if (const auto* BO = dyn_cast<BinaryOperator>(Flags)), so that you don't call cast<BinaryOperator>(Flags) multiple times below.

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

77

I would add tests where O_CLOEXEC is not at the end of parameter 2 expression like open("filename", O_RDWR | O_CLOEXEC | O_EXCL).

[yawanng]

clang-tidy/android/FileOpenFlagCheck.cpp

62

May I ask what's the difference between the Result.Context.getLangOpts() and getLangOpts()? Does Result.Context.getLangOpts() have a longer lifetime than getLangOpts()?

80

getImmediateMacroName sometimes cannot return the O_CLOEXEC , like

#define O_CLOEXEC _O_CLOEXEC
#define _O_CLOEXEC 1

getImmediateMacroName will return _O_CLOEXEC, whereas O_CLOEXEC is what we need. I change this to directly get the source text if it's a macro, which seems to be simpler.

[yawanng]

Format change.

[hokein]

The code looks good to me now, I'd wait to see whether @alexfh has further comments before submitting the patch. Thanks for your patience with the review ;)

clang-tidy/android/FileOpenFlagCheck.cpp

23

checkFlags name is a bit of ambiguous, maybe rename it to "HasCloseOnExecFlag"?

Notice that you use the string "O_CLOEXEC" several times in the code, I'd suggest to use a "CloseOnExecFlag" variable.

34

Nit: The outermost () is not needed.

62

IMO they are the same fundamentally, except Result.Context.getLangOpts() can save a copy. We usually use Result.Context.getLangOpts() inside ClangTidyCheck::check(const MatchFinder::MatchResult &Result).

80

Ah, I see, thanks for the clarification!

[alexfh]

LG with one nit.

clang-tidy/android/FileOpenFlagCheck.cpp

25

nit: "Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo())." (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)

clang-tidy/CMakeLists.txt

   clangToolingCore
   )
 
+add_subdirectory(android)
 add_subdirectory(boost)
 add_subdirectory(cert)
 add_subdirectory(cppcoreguidelines)

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 "FileOpenFlagCheck.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<FileOpenFlagCheck>("android-file-open-flag");
+  }
+};
+
+// Register the AndroidTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<AndroidModule>
+    X("android-module", "Adds Android platform checks.");
+
+} // namespace android
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the AndroidModule.
+volatile int AndroidModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tidy/android/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyAndroidModule
+  AndroidTidyModule.cpp
+  FileOpenFlagCheck.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )

clang-tidy/android/FileOpenFlagCheck.h

+//===--- FileOpenFlagCheck.h - clang-tidy----------------------------------===//
+//
+//                      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_FILE_OPEN_FLAG_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_FILE_OPEN_FLAG_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+/// Finds code that opens file without using the O_CLOEXEC flag.
+///
+/// open(), openat(), and open64() had better to include O_CLOEXEC in their
+/// flags argument. Only consider simple cases that the corresponding argument
+/// is constant or binary operation OR among constants like 'O_CLOEXEC' or
+/// 'O_CLOEXEC | O_RDONLY'. No constant propagation is performed.
+///
+/// Only the symbolic 'O_CLOEXEC' macro definition is checked, not the concrete
+/// value.
+class FileOpenFlagCheck : public ClangTidyCheck {
+public:
+  FileOpenFlagCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void doCheck(const ast_matchers::MatchFinder::MatchResult &Result,
+               const CallExpr *MatchedCall, const Expr *FlagArg,
+               const FunctionDecl *FD);
+  bool checkFlags(const Expr *Flags, const SourceManager &SM);
+
+  static constexpr const char *O_CLOEXEC = "O_CLOEXEC";
+};
+
+} // namespace android
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_FILE_OPEN_FLAG_H

clang-tidy/android/FileOpenFlagCheck.cpp

+//===--- FileOpenFlagCheck.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 "FileOpenFlagCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+void FileOpenFlagCheck::registerMatchers(MatchFinder *Finder) {
+  auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter())));
+
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(isExternC(), returns(isInteger()),
+                                   hasParameter(0, CharPointerType),
+                                   hasParameter(1, hasType(isInteger())),
+                                   hasAnyName("open", "open64"))
+                          .bind("funcDecl")))
+          .bind("openFn"),
+      this);
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(isExternC(), returns(isInteger()),
+                                   hasParameter(0, hasType(isInteger())),
+                                   hasParameter(1, CharPointerType),
+                                   hasParameter(2, hasType(isInteger())),
+                                   hasName("openat"))
+                          .bind("funcDecl")))
+          .bind("openatFn"),
+      this);
+}
+
+void FileOpenFlagCheck::check(const MatchFinder::MatchResult &Result) {
+  const CallExpr *MatchedCall;
+  const Expr *FlagArg;
+  if ((MatchedCall = Result.Nodes.getNodeAs<CallExpr>("openFn")))
+    FlagArg = MatchedCall->getArg(1);
+  else if ((MatchedCall = Result.Nodes.getNodeAs<CallExpr>("openatFn")))
+    FlagArg = MatchedCall->getArg(2);
+  else
+    return;
+
+  const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("funcDecl");
+
+  // Check the required flag.
+  doCheck(Result, MatchedCall, FlagArg, FD);
+}
+
+void FileOpenFlagCheck::doCheck(const MatchFinder::MatchResult &Result,
+                                const CallExpr *MatchedCall,
+                                const Expr *FlagArg, const FunctionDecl *FD) {
+  SourceManager &SM = *Result.SourceManager;
+
+  if (!checkFlags(FlagArg->IgnoreParenCasts(), SM)) {
+    LangOptions LangOpts = getLangOpts();
+    SourceRange FlagsRange(FlagArg->getLocStart(), FlagArg->getLocEnd());
+    StringRef FlagsText = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(FlagsRange), SM, LangOpts);
+    std::string ReplacementText =
+        (llvm::Twine(FlagsText) + " | " + O_CLOEXEC).str();
+    diag(FlagArg->getLocStart(), "%0 should use %1 where possible.")
+        << FD->getName() << O_CLOEXEC
+        << FixItHint::CreateReplacement(FlagsRange, ReplacementText);
+  }
+}
+
+// Check if flags contain required flag.
+// Args:
+//   Flags: The argument in the code.
+//   SM: The SourceManager.
+// Return:
+//   True if the required flag is declared in the argument.
+bool FileOpenFlagCheck::checkFlags(const Expr *Flags, const SourceManager &SM) {
+  bool IsFlagIn;
+  // If the Flag is an integer constant, check it.
+  if (isa<IntegerLiteral>(Flags)) {
+    SourceLocation SL = Flags->getLocStart();
+
+    if (!SM.isMacroBodyExpansion(SL))
+      return false;
+
+    // Get the Marco name.
+    LangOptions LangOpts = getLangOpts();
+    auto Loc = SM.getFileLoc(SL);
+    std::pair<FileID, unsigned> ExpansionInfo = SM.getDecomposedLoc(Loc);
+    unsigned MacroTokenLength = Lexer::MeasureTokenLength(Loc, SM, LangOpts);
+    StringRef ExpansionBuffer = SM.getBufferData(ExpansionInfo.first);
+    auto MacroName =
+        ExpansionBuffer.substr(ExpansionInfo.second, MacroTokenLength);
+
+    IsFlagIn = (MacroName == O_CLOEXEC);
+
+  }
+  // If it's a binary OR operation.
+  else if ((isa<BinaryOperator>(Flags)) &&
+           (cast<BinaryOperator>(Flags)->getOpcode() ==
+            clang::BinaryOperatorKind::BO_Or)) {
+    IsFlagIn =
+        checkFlags(cast<BinaryOperator>(Flags)->getLHS()->IgnoreParenCasts(),
+                   SM) ||
+        checkFlags(cast<BinaryOperator>(Flags)->getRHS()->IgnoreParenCasts(),
+                   SM);
+  }
+  // Otherwise, we assume it has the flag to avoid false positive.
+  else
+    IsFlagIn = true;
+
+  return IsFlagIn;
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang

clang-tidy/plugin/CMakeLists.txt

   clangFrontend
   clangSema
   clangTidy
+  clangTidyAndroidModule
   clangTidyBoostModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule

clang-tidy/tool/CMakeLists.txt

   clangASTMatchers
   clangBasic
   clangTidy
+  clangTidyAndroidModule
   clangTidyBoostModule
   clangTidyCERTModule
   clangTidyCppCoreGuidelinesModule

clang-tidy/tool/ClangTidyMain.cpp

 static int LLVM_ATTRIBUTE_UNUSED GoogleModuleAnchorDestination =
     GoogleModuleAnchorSource;
 
+// This anchor is used to force the linker to link the AndroidModule.
+extern volatile int AndroidModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED AndroidModuleAnchorDestination =
+    AndroidModuleAnchorSource;
+
 // This anchor is used to force the linker to link the MiscModule.
 extern volatile int MiscModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED MiscModuleAnchorDestination =

docs/ReleaseNotes.rst

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

docs/clang-tidy/checks/android-file-open-flag.rst

+.. title:: clang-tidy - android-file-open-flag
+
+android-file-open-flag
+======================
+
+A common source of security bugs has been code that opens file without using
+the ``O_CLOEXEC`` flag.  Without that flag, an opened sensitive file would
+remain open across a fork+exec to a lower-privileged SELinux domain, leaking
+that sensitive data Functions including ``open()``, ``openat()``, and
+``open64()`` must include ``O_CLOEXEC`` in their flags argument.
+
+Examples:
+
+.. code-block:: c++
+
+  open("filename", O_RDWR);
+  open64("filename", O_RDWR);
+  openat(0, "filename", O_RDWR);
+
+  // becomes
+
+  open("filename", O_RDWR | O_CLOEXEC);
+  open64("filename", O_RDWR | O_CLOEXEC);
+  openat(0, "filename", O_RDWR | O_CLOEXEC);

docs/clang-tidy/checks/list.rst

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

docs/clang-tidy/index.rst

 ====================== =========================================================
 Name prefix            Description
 ====================== =========================================================
+``android``
 ``boost-``             Checks related to Boost library.
 ``cert-``              Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.

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]]:20: 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]]:20: 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]]:22: 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]]:22: 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]]:25: 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]]:25: warning: openat should use O_CLOEXEC where
+  // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC
+}
+
+void f() {
+  open("filename", 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible. [android-file-open-flag]
+  // CHECK-FIXES: 3 | O_CLOEXEC
+  open64("filename", 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: 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]]:25: 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:
+  open64("filename", O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  open64("filename", O_RDWR | O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  openat(0, "filename", O_CLOEXEC);
+  // CHECK-MESSAGES-NOT: warning:
+  openat(0, "filename", O_RDWR | O_CLOEXEC);
+  // 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:
+  }
+};

unittests/clang-tidy/CMakeLists.txt

   clangFrontend
   clangLex
   clangTidy
+  clangTidyAndroidModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule