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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
