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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:60
+
+  LangOptions LangOpts = getLangOpts();
+  SourceRange FlagsRange(FlagArg->getLocStart(), FlagArg->getLocEnd());
----------------
No need for this variable, since it's just used once. Same below.


================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:62-65
+  StringRef FlagsText = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(FlagsRange), SM, LangOpts);
+  std::string ReplacementText =
+      (llvm::Twine(FlagsText) + " | " + O_CLOEXEC).str();
----------------
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.


================
Comment at: clang-tidy/android/FileOpenFlagCheck.h:38
+
+  static constexpr const char *O_CLOEXEC = "O_CLOEXEC";
+};
----------------
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.


================
Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:6
+
+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
----------------
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."


================
Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:9
+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.
----------------
Missing period before "Functions".


================
Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:9
+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.
----------------
alexfh wrote:
> Missing period before "Functions".
Just "functions" is too broad. I'd say "`open`-like functions", "POSIX functions that open files" or something like that.


https://reviews.llvm.org/D33304