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

[jcai19]

Summary

pipe2() is better to set O_CLOEXEC flag to avoid file descriptor leakage.

[Eugene.Zelenko]

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

1 ↗

(On Diff #199951)

Please extend to 80 characters. Same in source file.

[jcai19]

Update based on comments.

[srhines]

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

19 ↗

(On Diff #200066)

Shouldn't this be "O_NONBLOCK | O_CLOEXEC" instead? Why drop the O_NONBLOCK?

[jcai19]

Fix a typo.

[jcai19]

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

19 ↗

(On Diff #200066)

Good catch!

[srhines]

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

52 ↗

(On Diff #200066)

I'm not all that familiar with writing clang-tidy-specific tests, but should these tests here denote that a diagnostic should NOT be issued? That is usually the convention in regular Clang tests, so I assume the test runner here should be equally supportive of ensuring that the contents passed through without any other diagnostics related to pipe2 and/or O_CLOEXEC.

[jcai19]

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

52 ↗

(On Diff #200066)

That makes sense, and I have seem tests for similar checks with (e.g. android-cloexec-open) and without (e.g. android-cloexec-accep4 and android-cloexec-socket) additional CHECK-MESSAGES-NOT check. But based on the Testing Checks section of https://clang.llvm.org/extra/clang-tidy/Contributing.html, it seems typically CHECK-MASSAGES and CHECK-FIXES are sufficient for clang-tidy checks. Please let me know what you think.

[srhines]

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

52 ↗

(On Diff #200066)

If you look in test/clang-tidy/android-cloexec-creat.cpp, you will see that there are "CHECK-MESSAGES-NOT" checks that ensure the diagnostic is not issued in correct cases. You can put the checks on lines 39, 58, and 67, which will ensure that there are no additional diagnostics being generated.

[jcai19]

Add CHECK-MESSAGES-NOT checks.

[jcai19]

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

52 ↗

(On Diff #200066)

Sounds good! Thanks for the reference.

[srhines]

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

48 ↗

(On Diff #202030)

Much like line 39 (which covers both lines 37 and 38), you can delete this CHECK-MESSAGES-NOT. The one on line 50 will cover both of these.

64 ↗

(On Diff #202030)

Only keep this CHECK-MESSAGES-NOT line.

75 ↗

(On Diff #202030)

Only keep this CHECK-MESSAGES-NOT line.

[jcai19]

Remove redundant CHECK-MESSAGES-NOT checks.

[srhines]

Everything looks great. Thanks for adding these improvements. While it's probably safe to commit this, perhaps you should give it 24 hours in case some of the other clang-tidy folks have different style or testing concerns.

[jcai19]

In D62049#1522203, @srhines wrote:

Everything looks great. Thanks for adding these improvements. While it's probably safe to commit this, perhaps you should give it 24 hours in case some of the other clang-tidy folks have different style or testing concerns.

Sounds good! Thanks for all the comments.

[hokein]

LGTM.

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

60 ↗

(On Diff #202072)

no need to verify it explicitly.

[gribozavr]

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

6 ↗

(On Diff #202072)

This checks ensures that pipe2() is called with the O_CLOEXEC flag. This flag helps to avoid accidentally leaking file descriptors to child processes.

17 ↗

(On Diff #202072)

"Suggested replacement"

Also use a separate code block for the replacement.

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

20 ↗

(On Diff #202072)

Same comment about the message as in D61967 -- the message should briefly explain why the user should make this change. Users tend to ignore warnings they don't understand.

"pipe2 should be called with O_CLOEXEC to avoid leaking file descriptors to child processes"

27 ↗

(On Diff #202072)

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

54 ↗

(On Diff #202072)

How is e different from a?

48 ↗

(On Diff #202030)

Not done -- you don't need any CHECK-MESSAGES-NOT at all.

[srhines]

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

54 ↗

(On Diff #202072)

e uses O_CLOEXEC properly with pipe2() and makes sure that we don't issue additional diagnostics/fixes that are unnecessary.

[gribozavr]

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

54 ↗

(On Diff #202072)

Thanks -- I see it now. I think a descriptive test name would help here.

[jcai19]

Update description based on comments and remove CHECK-MESSAGES-NOT

[jcai19]

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

20 ↗

(On Diff #202072)

It appeared to me that there was no easy way to change the warning message due to the way insertMacroFlag is implemented (called in CloexecPipe2Check::check(...) to add O_CLOEXEC flag when necessary). The function always issue a warning in the format of "%0 should use %1 where possible". So unless we parameterize the warning string (which could be solved by a different code change), we might end up implementing a similar function with different warning message, which creates extra work if we need to modify insertMacroFlag in the future. I am new to Clang so please let me know if there is a better way to udpate the warning message.

[jcai19]

Fix a typo.

[gribozavr]

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

17 ↗

(On Diff #202832)

warning1, warning2 is better than a and b, but we can do even better.

warningForPipeCall
warningForPipeCallInMacroArgument

Similarly in the rest of the patch.

20 ↗

(On Diff #202072)

I don't see an issue with parameterizing insertMacroFlag -- the current message that it produces is not descriptive enough anyway, users tend to ignore such messages.

[jcai19]

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

20 ↗

(On Diff #202072)

I agree. But my point is insertMacroFlag is used by several other checks. Shouldn't we parameterize the warning message and update these checks together in a separate change?

[gribozavr]

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

20 ↗

(On Diff #202072)

SGTM!

[jcai19]

Update test function names.

[jcai19]

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

17 ↗

(On Diff #202832)

Sounds good. I have updated the function names, although I feel the "ForPipeCall" part is already implied in the test so I did not include it to keep the names shorter. Let me know if you think it is better to have it included.

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 "CloexecPipe2Check.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<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.
 static ClangTidyModuleRegistry::Add<AndroidModule>

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
+  CloexecPipe2Check.cpp
   CloexecSocketCheck.cpp
   ComparisonInTempFailureRetryCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.h

+//===--- CloexecPipe2Check.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_PIPE2_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE2_H
+
+#include "CloexecCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+/// Finds code that uses pipe2() without using the O_CLOEXEC flag.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-pipe2.html
+class CloexecPipe2Check : public CloexecCheck {
+public:
+  CloexecPipe2Check(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_PIPE2_H

clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.cpp

+//===--- CloexecPipe2Check.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 "CloexecPipe2Check.h"
+#include "../utils/ASTUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+void CloexecPipe2Check::registerMatchers(MatchFinder *Finder) {
+  registerMatchersImpl(Finder,
+                       functionDecl(returns(isInteger()), hasName("pipe2"),
+                                    hasParameter(0, hasType(pointsTo(isInteger()))),
+                                    hasParameter(1, hasType(isInteger()))));
+}
+
+void CloexecPipe2Check::check(const MatchFinder::MatchResult &Result) {
+  insertMacroFlag(Result, /*MacroFlag=*/"O_CLOEXEC", /*ArgPos=*/1);
+}
+
+} // 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-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.
 
   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/trunk/docs/clang-tidy/checks/android-cloexec-pipe2.rst

+.. title:: clang-tidy - android-cloexec-pipe2
+
+android-cloexec-pipe2
+=====================
+
+This checks ensures that pipe2() is called with the O_CLOEXEC flag. 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++
+
+  pipe2(pipefd, O_NONBLOCK);
+
+Suggested replacement:
+
+.. code-block:: c++
+
+  pipe2(pipefd, O_NONBLOCK | 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-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
    bugprone-copy-constructor-init

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

+// RUN: %check_clang_tidy %s android-cloexec-pipe2 %t
+
+#define O_NONBLOCK 1
+#define __O_CLOEXEC 3
+#define O_CLOEXEC __O_CLOEXEC
+#define TEMP_FAILURE_RETRY(exp) \
+  ({                            \
+    int _rc;                    \
+    do {                        \
+      _rc = (exp);              \
+    } while (_rc == -1);        \
+  })
+#define NULL 0
+
+extern "C" int pipe2(int pipefd[2], int flags);
+
+void warning() {
+  int pipefd[2];
+  pipe2(pipefd, O_NONBLOCK);
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 'pipe2'
+  // CHECK-FIXES: pipe2(pipefd, O_NONBLOCK | O_CLOEXEC);
+  TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK));
+  // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: 'pipe2'
+  // CHECK-FIXES: TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK | O_CLOEXEC));
+}
+
+void warningInMacroArugment() {
+  int pipefd[2];
+  pipe2(pipefd, 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'pipe2'
+  // CHECK-FIXES: pipe2(pipefd, 3 | O_CLOEXEC);
+  TEMP_FAILURE_RETRY(pipe2(pipefd, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'pipe2'
+  // CHECK-FIXES: TEMP_FAILURE_RETRY(pipe2(pipefd, 3 | O_CLOEXEC));
+
+  int flag = O_NONBLOCK;
+  pipe2(pipefd, flag);
+  TEMP_FAILURE_RETRY(pipe2(pipefd, flag));
+}
+
+namespace i {
+int pipe2(int pipefd[2], int flags);
+
+void noWarning() {
+  int pipefd[2];
+  pipe2(pipefd, O_NONBLOCK);
+  TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK));
+}
+
+} // namespace i
+
+void noWarning() {
+  int pipefd[2];
+  pipe2(pipefd, O_CLOEXEC);
+  TEMP_FAILURE_RETRY(pipe2(pipefd, O_CLOEXEC));
+  pipe2(pipefd, O_NONBLOCK | O_CLOEXEC);
+  TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK | O_CLOEXEC));
+}
+
+class G {
+public:
+  int pipe2(int pipefd[2], int flags);
+  void noWarning() {
+    int pipefd[2];
+    pipe2(pipefd, O_NONBLOCK);
+    TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK));
+  }
+};