[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

22 ↗

(On Diff #199692)

We probably don't want to commit commented out code

27 ↗

(On Diff #199692)

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

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

18 ↗

(On Diff #199692)

nit: should probably update this comment

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

9 ↗

(On Diff #199692)

(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

6 ↗

(On Diff #199692)

Please make first sentence same as in Release Notes.

8 ↗

(On Diff #199692)

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

4 ↗

(On Diff #199692)

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

27 ↗

(On Diff #199692)

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

18 ↗

(On Diff #199692)

Good catch!

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

4 ↗

(On Diff #199692)

Thanks for the comments. I have fixed them accordingly.

[jcai19]

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

9 ↗

(On Diff #199692)

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

27 ↗

(On Diff #199692)

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

27 ↗

(On Diff #199692)

Make sense.

[Eugene.Zelenko]

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

1 ↗

(On Diff #199926)

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

31 ↗

(On Diff #202052)

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

17 ↗

(On Diff #202052)

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

31 ↗

(On Diff #202052)

"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

18 ↗

(On Diff #202052)

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

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

6 ↗

(On Diff #202052)

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

16 ↗

(On Diff #202052)

"Suggested replacement"

Also use a separate code block for the replacement.

[srhines]

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

17 ↗

(On Diff #202052)

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

31 ↗

(On Diff #202052)

Thanks for the comments. I have updated the text.

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

17 ↗

(On Diff #202052)

Thanks for the clarification.

[gribozavr]

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

5 ↗

(On Diff #202052)

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

14 ↗

(On Diff #202835)

noWarningForPipeInNamespace ?

23 ↗

(On Diff #202835)

noWarningForPipeMemberFunction ?

[jcai19]

Update test function names.

clang-tools-extra/trunk/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 "CloexecPipe2Check.h"
 #include "CloexecSocketCheck.h"
 #include "ComparisonInTempFailureRetryCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
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<CloexecPipe2Check>("android-cloexec-pipe2");
     CheckFactories.registerCheck<CloexecSocketCheck>("android-cloexec-socket");
     CheckFactories.registerCheck<ComparisonInTempFailureRetryCheck>(
         "android-comparison-in-temp-failure-retry");
   }
 };
 
 // Register the AndroidTidyModule using this statically initialized variable.

clang-tools-extra/trunk/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
   CloexecPipe2Check.cpp
   CloexecSocketCheck.cpp
   ComparisonInTempFailureRetryCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/trunk/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 {
+
+/// Suggests to replace calls to pipe() with calls to 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/trunk/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) {
+  std::string ReplacementText =
+      (Twine("pipe2(") + getSpellingArg(Result, 0) + ", O_CLOEXEC)").str();
+
+  replaceFunc(
+      Result,
+      "prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes",
+      ReplacementText);
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/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.
+
+  This check detects usage of ``pipe()``.
+
 - New :doc:`android-cloexec-pipe2
   <clang-tidy/checks/android-cloexec-pipe2>` check.
 
   This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
 
 - New :doc:`bugprone-unhandled-self-assignment
   <clang-tidy/checks/bugprone-unhandled-self-assignment>` check.
 

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

+.. title:: clang-tidy - android-cloexec-pipe
+
+android-cloexec-pipe
+====================
+
+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.
+
+Examples:
+
+.. code-block:: c++
+
+  pipe(pipefd);
+
+Suggested replacement:
+
+.. code-block:: c++
+  pipe2(pipefd, O_CLOEXEC);

clang-tools-extra/trunk/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-pipe2
    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

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

+// RUN: %check_clang_tidy %s android-cloexec-pipe %t
+
+extern "C" int pipe(int pipefd[2]);
+
+void warning() {
+  int pipefd[2];
+  pipe(pipefd);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes [android-cloexec-pipe]
+  // CHECK-FIXES: pipe2(pipefd, O_CLOEXEC);
+}
+
+namespace i {
+int pipe(int pipefd[2]);
+void noWarningInNamespace() {
+  int pipefd[2];
+  pipe(pipefd);
+}
+} // namespace i
+
+class C {
+public:
+  int pipe(int pipefd[2]);
+  void noWarningForMemberFunction() {
+    int pipefd[2];
+    pipe(pipefd);
+  }
+};