[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:31
+      Result,
+      "prefer pipe2() to pipe() because pipe2() allows O_CLOEXEC",
+      ReplacementText);
----------------
hokein wrote:
> the message doesn't seem to explain the reason, especially to the people who are not familiar with the `pipe` API, maybe `prefer pipe2() to pipe() to avoid file descriptor leakage` is clearer?
> 
> Ah, it looks like you are following the existing `CloexecCreatCheck` check, no need to do it in this patch. 
"prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes"

No need to change in this patch if you're following an existing pattern, but please do update the message in all checks in a separate patch to explain things better.


================
Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h:18
+
+/// pipe() is better to be replaced by pipe2().
+///
----------------
"Suggests to replace calls to pipe() with calls to pipe2()."


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst:6
+
+Detects usage of ``pipe()``. The usage of ``pipe()`` is not recommended, it's better to use ``pipe2()``.
+Without this flag, an opened sensitive file descriptor would remain open across
----------------
WDYT about this rewrite?

"""
This check detects usage of pipe().  Using pipe() is not recommended, pipe2() is the suggested replacement.

The check also adds the O_CLOEXEC flag that marks the file descriptor to be closed in child processes.  Without this flag a sensitive file descriptor can be leaked to a child process, potentially into a lower-privileged SELinux domain.
"""


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst:16
+
+  // becomes
+
----------------
"Suggested replacement"

Also use a separate code block for the replacement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61967/new/

https://reviews.llvm.org/D61967