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

[jcai19]

Summary

pipe() is better to be replaced by pipe2() with O_CLOEXEC flag to avoid file descriptor leakage.

[george.burgess.iv]

Thanks for this!

I don't have great context on tidy, so I can't stamp this, but I do have a few drive-by nits for you.

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp

23

We probably don't want to commit commented out code

28

simplicity nit: can this be a std::string?

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h

19

nit: should probably update this comment

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp

10

(Do we have a CHECK-FIXES-NOT or CHECK-MESSAGES-NOT to apply to the things below? Or are the CHECKs here meant to be complete like clang's -verify?)

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst

7

Please make first sentence same as in Release Notes.

9

I think will be good idea to highlight fork and exec with double back-ticks.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst

5

Please make same length as name above.

[jcai19]

Add fixes based on comments

[jcai19]

Fix format

[jcai19]

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp

28

replaceFunc takes a llvm::StringRef as the third parameter. StringRef is "expected to be used in situations where the character data resides in some other buffer, whose lifetime extends past that of the StringRef", which is true in this case, so I think it should be fine.

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h

19

Good catch!

clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst

5

Thanks for the comments. I have fixed them accordingly.

[jcai19]

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp

10

Based on Testing Checks https://clang.llvm.org/extra/clang-tidy/Contributing.html, CHECK-MASSAGES and CHECK-FIXES are sufficient for clang-tidy checks. I have double checked the tests for similar checks and they don't seem to have additional FileCheck invocations other than these two.

[george.burgess.iv]

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp

28

Both should be functionally equivalent in this code. My point was that it's not common to rely on temporary lifetime extension when writing the non-const& type would be equivalent (barring maybe cases with auto, but that's not applicable), and I don't see why we should break with that here.

[jcai19]

Use non-const& type for readbility.

[jcai19]

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp

28

Make sense.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h

2

Please extend to 80 characters. Same in source file.

[jcai19]

Update based on comments.

[jcai19]

Add CHECK-MESSAGES-NOT checks.

[hokein]

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp

32

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.

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp

18

nit: no need to do it explicitly, if a warning is shown unexpectedly, the test will fail.

[gribozavr]

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp

32

"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.

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h

19

"Suggests to replace calls to pipe() with calls to pipe2()."

clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst

7

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.
"""

17

"Suggested replacement"

Also use a separate code block for the replacement.

[srhines]

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp

18

I somehow never realized this (and there seem to be several places that also don't realize it). Thanks for letting us know.

[jcai19]

Remove CHECK-MESSAGES-NOT and update description based on comments

[jcai19]

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp

32

Thanks for the comments. I have updated the text.

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp

18

Thanks for the clarification.

[gribozavr]

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp

6

Please give the tests informative names instead of f, g etc.

[jcai19]

Update the test function names.

[jcai19]

Thanks for all the comments. george.burgess.iv will submit this change for me.

[george.burgess.iv]

Will submit once gribozavr indicates that they're happy with the new test names. Thanks again for working on this!

[gribozavr]

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp

15

noWarningForPipeInNamespace ?

24

noWarningForPipeMemberFunction ?

[jcai19]

Update test function names.

clang-tools-extra/clang-tidy/android/AndroidTidyModule.cpp

 #include "CloexecDupCheck.h"
 #include "CloexecEpollCreate1Check.h"
 #include "CloexecEpollCreateCheck.h"
 #include "CloexecFopenCheck.h"
 #include "CloexecInotifyInit1Check.h"
 #include "CloexecInotifyInitCheck.h"
 #include "CloexecMemfdCreateCheck.h"
 #include "CloexecOpenCheck.h"
+#include "CloexecPipeCheck.h"
 #include "CloexecSocketCheck.h"
 #include "ComparisonInTempFailureRetryCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace android {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<CloexecFopenCheck>("android-cloexec-fopen");
     CheckFactories.registerCheck<CloexecInotifyInit1Check>(
         "android-cloexec-inotify-init1");
     CheckFactories.registerCheck<CloexecInotifyInitCheck>(
         "android-cloexec-inotify-init");
     CheckFactories.registerCheck<CloexecMemfdCreateCheck>(
         "android-cloexec-memfd-create");
     CheckFactories.registerCheck<CloexecOpenCheck>("android-cloexec-open");
+    CheckFactories.registerCheck<CloexecPipeCheck>("android-cloexec-pipe");
     CheckFactories.registerCheck<CloexecSocketCheck>("android-cloexec-socket");
     CheckFactories.registerCheck<ComparisonInTempFailureRetryCheck>(
         "android-comparison-in-temp-failure-retry");
   }
 };
 
 // Register the AndroidTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<AndroidModule>

clang-tools-extra/clang-tidy/android/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAndroidModule
   AndroidTidyModule.cpp
   CloexecAccept4Check.cpp
   CloexecAcceptCheck.cpp
   CloexecCheck.cpp
   CloexecCreatCheck.cpp
   CloexecDupCheck.cpp
   CloexecEpollCreate1Check.cpp
   CloexecEpollCreateCheck.cpp
   CloexecFopenCheck.cpp
   CloexecInotifyInit1Check.cpp
   CloexecInotifyInitCheck.cpp
   CloexecMemfdCreateCheck.cpp
   CloexecOpenCheck.cpp
+  CloexecPipeCheck.cpp
   CloexecSocketCheck.cpp
   ComparisonInTempFailureRetryCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h

+//===--- CloexecPipeCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE_H
+
+#include "CloexecCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+/// pipe() is better to be replaced by pipe2().
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-pipe.html
+class CloexecPipeCheck : public CloexecCheck {
+public:
+  CloexecPipeCheck(StringRef Name, ClangTidyContext *Context)
+      : CloexecCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace android
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE_H

clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp

+//===--- CloexecPipeCheck.cpp - clang-tidy-------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CloexecPipeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+void CloexecPipeCheck::registerMatchers(MatchFinder *Finder) {
+  registerMatchersImpl(Finder,
+                       functionDecl(returns(isInteger()), hasName("pipe"),
+                                    hasParameter(0, hasType(pointsTo(isInteger())))));
+}
+
+void CloexecPipeCheck::check(const MatchFinder::MatchResult &Result) {
+  const std::string &ReplacementText =
+      (Twine("pipe2(") + getSpellingArg(Result, 0) + ", O_CLOEXEC)").str();
+
+  replaceFunc(
+      Result,
+      "prefer pipe2() to pipe() because pipe2() allows O_CLOEXEC",
+      ReplacementText);
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`abseil-time-comparison
   domain.
 
 - New :doc:`abseil-time-subtraction
   <clang-tidy/checks/abseil-time-subtraction>` check.
 
   Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
   in the Time domain instead of the numeric domain.
 
+- New :doc:`android-cloexec-pipe
+  <clang-tidy/checks/android-cloexec-pipe>` check.
+
+  Detects usage of ``pipe()``.
+
 - New :doc:`bugprone-unhandled-self-assignment
   <clang-tidy/checks/bugprone-unhandled-self-assignment>` check.
 
   Finds user-defined copy assignment operators which do not protect the code
   against self-assignment either by checking self-assignment explicitly or
   using the copy-and-swap or the copy-and-move method.
 
 - New :doc:`bugprone-branch-clone

clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst

+.. title:: clang-tidy - android-cloexec-pipe
+
+android-cloexec-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
+a fork+exec to a lower-privileged SELinux domain.
+
+Examples:
+
+.. code-block:: c++
+
+  pipe(pipefd);
+
+  // becomes
+
+  pipe2(pipefd, O_CLOEXEC);

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. toctree::
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen
    android-cloexec-inotify-init
    android-cloexec-inotify-init1
    android-cloexec-memfd-create
    android-cloexec-open
+   android-cloexec-pipe
    android-cloexec-socket
    android-comparison-in-temp-failure-retry
    boost-use-to-string
    bugprone-argument-comment
    bugprone-assert-side-effect
    bugprone-bool-pointer-implicit-conversion
    bugprone-branch-clone
    bugprone-copy-constructor-init

clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp

+// RUN: %check_clang_tidy %s android-cloexec-pipe %t
+
+extern "C" int pipe(int pipefd[2]);
+
+void f() {
+  int pipefd[2];
+  pipe(pipefd);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer pipe2() to pipe() because pipe2() allows O_CLOEXEC [android-cloexec-pipe]
+  // CHECK-FIXES: pipe2(pipefd, O_CLOEXEC);
+}
+
+namespace i {
+int pipe(int pipefd[2]);
+void g() {
+  int pipefd[2];
+  pipe(pipefd);
+}
+} // namespace i
+
+class C {
+public:
+  int pipe(int pipefd[2]);
+  void h() {
+    int pipefd[2];
+    pipe(pipefd);
+  }
+};