[clang-tidy] Add a new Android check "android-cloexec-socket"

[yawanng]

Summary

socket() is better to include SOCK_CLOEXEC in its type argument to avoid the file descriptor leakage.

[chh]

LGTM. If nobody needs this during the long weekend, it's better to submit after the long weekend.

[Eugene.Zelenko]

Please wait for Alexander, Aaron or Haojian approval.

clang-tidy/android/CloexecOpenCheck.cpp

22 ↗

(On Diff #104946)

Anonymous namespaces should contain only class declarations. static is enough.

clang-tidy/android/CloexecSocketCheck.cpp

21 ↗

(On Diff #104946)

Anonymous namespaces should contain only class declarations. static is enough.

docs/ReleaseNotes.rst

79 ↗

(On Diff #104946)

Maybe present in arguments of instead of exist will sound better?

docs/clang-tidy/checks/android-cloexec-socket.rst

5 ↗

(On Diff #104946)

Please add empty line there.

[hokein]

clang-tidy/utils/CloexecFlagChecker.h

21 ↗

(On Diff #104946)

Maybe put this function to utils/ASTUtils.h file.

The name CloseOnExecFlag is too specific, it actually checks whether the flags expr has a given flag IMO, might be call it hasFlag (a better name are welcome), and document more details of the API in the above comment.

test/clang-tidy/android-cloexec-socket.cpp

20 ↗

(On Diff #104946)

I'd use the complete statement for checking the fixes here, the same below.

92 ↗

(On Diff #104946)

nit: remove the empty line.

[alexfh]

test/clang-tidy/android-cloexec-socket.cpp

20 ↗

(On Diff #104946)

Yes, CHECK-FIXES has fewer context than CHECK-MESSAGES (it doesn't have the line number, for example), and there's much more stuff that can go wrong than with the static diagnostic messages. So please make CHECK-FIXES as specific, as possible. Ideally, they should be unique, so that a wrong line can't be matched by a CHECK-FIXES. If needed, add unique comments on identical lines.

[alexfh]

clang-tidy/utils/ASTUtils.h

24 ↗

(On Diff #105293)

s/presents/is present/

27 ↗

(On Diff #105293)

hasFlag is too generic and may give a totally wrong impression of what the function does. Even in the context of this utility library I would prefer to expand the name and make it less ambiguous. Something along the lines of exprHasBitFlagWithSpelling. I wouldn't also insist on the flag being implemented as a macro, since this restriction would prevent many valid use cases with enum or constexpr int flags.

28 ↗

(On Diff #105293)

CloseOnExecFlag is too specific for a function that is meant to be somewhat generic.

docs/ReleaseNotes.rst

79 ↗

(On Diff #105293)

s/presents/is present/

test/clang-tidy/android-cloexec-socket.cpp

65 ↗

(On Diff #105293)

// CHECK-MESSAGES-NOT: warning: is redundant. The test script runs FileCheck with -implicit-check-not={{warning:|error:}}, which takes care of stray warning: lines.

20 ↗

(On Diff #104946)

This is not addressed yet. The test contains a number of ambiguous CHECK-FIXES patterns. Please make each pattern (and the line being matched) unique to avoid incorrect matches. One way to do this is to add unique trailing comments to each of the similar lines and include this comment into the pattern.

[yawanng]

clang-tidy/utils/ASTUtils.h

27 ↗

(On Diff #105293)

Yes, the name is a little misleading and only consider the macro is not thorough. But for the purpose of the current Android checks, where this function is used, this simple solution can cover about 95% cases. Few use enum or constexpr for flags like O_CLOEXEC or other similar ones. Moreover, if extending the function to cover the constant integer variables, it's not easy to get the value of the checked flag, which may depend on the target. Generally, in current stage, this function works well in the Android cases and it's still a TODO for other more sophisticated user cases.

[alexfh]

clang-tidy/utils/ASTUtils.h

24 ↗

(On Diff #105878)

nit: Third-person singular form is better for interface comments ("<the function> Checks whether a macro flag .... <the function> Only considers cases of single match ....")

28 ↗

(On Diff #105878)

StringRef is a better type for read-only string arguments.

28 ↗

(On Diff #105878)

FlagSpelling or FlagName could be better than just Flag.

docs/clang-tidy/checks/android-cloexec-socket.rst

6 ↗

(On Diff #105878)

A bit more expanded explanation on where the descriptor can leak would make the documentation much better. If there's a good external information source describing this issue, you can also link there.

[alexfh]

LG

[yawanng]

In D34913#797577, @Eugene.Zelenko wrote:

Please wait for Alexander, Aaron or Haojian approval.

Could you please check this CL? Thank you.

[Eugene.Zelenko]

In D34913#806799, @yawanng wrote:

In D34913#797577, @Eugene.Zelenko wrote:

Please wait for Alexander, Aaron or Haojian approval.

Could you please check this CL? Thank you.

You could commit changes, since Alexander approved them.

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

 //===--- AndroidTidyModule.cpp - clang-tidy--------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "CloexecCreatCheck.h"
 #include "CloexecFopenCheck.h"
 #include "CloexecOpenCheck.h"
+#include "CloexecSocketCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace android {
 
 /// This module is for Android specific checks.
 class AndroidModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<CloexecCreatCheck>("android-cloexec-creat");
     CheckFactories.registerCheck<CloexecFopenCheck>("android-cloexec-fopen");
     CheckFactories.registerCheck<CloexecOpenCheck>("android-cloexec-open");
+    CheckFactories.registerCheck<CloexecSocketCheck>("android-cloexec-socket");
   }
 };
 
 // Register the AndroidTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<AndroidModule>
     X("android-module", "Adds Android platform checks.");
 
 } // namespace android
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the AndroidModule.
 volatile int AndroidModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAndroidModule
   AndroidTidyModule.cpp
   CloexecCreatCheck.cpp
   CloexecFopenCheck.cpp
   CloexecOpenCheck.cpp
+  CloexecSocketCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

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

 //===--- CloexecOpenCheck.cpp - clang-tidy---------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CloexecOpenCheck.h"
+#include "../utils/ASTUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace android {
 
-namespace {
 static constexpr const char *O_CLOEXEC = "O_CLOEXEC";
 
-bool hasCloseOnExecFlag(const Expr *Flags, const SourceManager &SM,
-                        const LangOptions &LangOpts) {
-  // If the Flag is an integer constant, check it.
-  if (isa<IntegerLiteral>(Flags)) {
-    if (!SM.isMacroBodyExpansion(Flags->getLocStart()) &&
-        !SM.isMacroArgExpansion(Flags->getLocStart()))
-      return false;
-
-    // Get the Marco name.
-    auto MacroName = Lexer::getSourceText(
-        CharSourceRange::getTokenRange(Flags->getSourceRange()), SM, LangOpts);
-
-    return MacroName == O_CLOEXEC;
-  }
-  // If it's a binary OR operation.
-  if (const auto *BO = dyn_cast<BinaryOperator>(Flags))
-    if (BO->getOpcode() == clang::BinaryOperatorKind::BO_Or)
-      return hasCloseOnExecFlag(BO->getLHS()->IgnoreParenCasts(), SM,
-                                LangOpts) ||
-             hasCloseOnExecFlag(BO->getRHS()->IgnoreParenCasts(), SM, LangOpts);
-
-  // Otherwise, assume it has the flag.
-  return true;
-}
-} // namespace
-
 void CloexecOpenCheck::registerMatchers(MatchFinder *Finder) {
   auto CharPointerType = hasType(pointerType(pointee(isAnyCharacter())));
 
   Finder->addMatcher(
       callExpr(callee(functionDecl(isExternC(), returns(isInteger()),
                                    hasAnyName("open", "open64"),
                                    hasParameter(0, CharPointerType),
                                    hasParameter(1, hasType(isInteger())))
else if (const auto *OpenFnCall =
                Result.Nodes.getNodeAs<CallExpr>("openatFn"))
     FlagArg = OpenFnCall->getArg(2);
   assert(FlagArg);
 
   const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("funcDecl");
 
   // Check the required flag.
   SourceManager &SM = *Result.SourceManager;
-  if (hasCloseOnExecFlag(FlagArg->IgnoreParenCasts(), SM,
-                         Result.Context->getLangOpts()))
+  if (utils::exprHasBitFlagWithSpelling(FlagArg->IgnoreParenCasts(), SM,
+                     Result.Context->getLangOpts(), O_CLOEXEC))
     return;
 
   SourceLocation EndLoc =
       Lexer::getLocForEndOfToken(SM.getFileLoc(FlagArg->getLocEnd()), 0, SM,
                                  Result.Context->getLangOpts());
 
   diag(EndLoc, "%0 should use %1 where possible")
       << FD << O_CLOEXEC
       << FixItHint::CreateInsertion(EndLoc, (Twine(" | ") + O_CLOEXEC).str());
 }
 
 } // namespace android
 } // namespace tidy
 } // namespace clang

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

+//===--- CloexecSocketCheck.h - clang-tidy-----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_SOCKET_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+/// Finds code that uses socket() without using the SOCK_CLOEXEC flag.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html
+class CloexecSocketCheck : public ClangTidyCheck {
+public:
+  CloexecSocketCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(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_SOCKET_H

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

+//===--- CloexecSocketCheck.cpp - clang-tidy-------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "CloexecSocketCheck.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 {
+
+static constexpr const char *SOCK_CLOEXEC = "SOCK_CLOEXEC";
+
+void CloexecSocketCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(isExternC(), returns(isInteger()),
+                                   hasName("socket"),
+                                   hasParameter(0, hasType(isInteger())),
+                                   hasParameter(1, hasType(isInteger())),
+                                   hasParameter(2, hasType(isInteger())))
+                          .bind("funcDecl")))
+          .bind("socketFn"),
+      this);
+}
+
+void CloexecSocketCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCall = Result.Nodes.getNodeAs<CallExpr>("socketFn");
+  const auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("funcDecl");
+  const Expr *FlagArg = MatchedCall->getArg(1);
+  SourceManager &SM = *Result.SourceManager;
+
+  if (utils::exprHasBitFlagWithSpelling(FlagArg->IgnoreParenCasts(), SM,
+                     Result.Context->getLangOpts(), SOCK_CLOEXEC))
+    return;
+
+  SourceLocation EndLoc =
+      Lexer::getLocForEndOfToken(SM.getFileLoc(FlagArg->getLocEnd()), 0, SM,
+                                 Result.Context->getLangOpts());
+
+  diag(EndLoc, "%0 should use %1 where possible")
+      << FD << SOCK_CLOEXEC
+      << FixItHint::CreateInsertion(EndLoc,
+                                    (Twine(" | ") + SOCK_CLOEXEC).str());
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/utils/ASTUtils.h

 namespace clang {
 namespace tidy {
 namespace utils {
 // Returns the (closest) Function declaration surrounding |Statement| or NULL.
 const FunctionDecl *getSurroundingFunction(ASTContext &Context,
                                            const Stmt &Statement);
 // Determine whether Expr is a Binary or Ternary expression.
 bool IsBinaryOrTernary(const Expr *E);
+
+/// Checks whether a macro flag is present in the given argument. Only considers
+/// cases of single match or match in a binary OR expression. For example,
+/// <needed-flag> or <flag> | <needed-flag> | ...
+bool exprHasBitFlagWithSpelling(const Expr *Flags, const SourceManager &SM,
+                                const LangOptions &LangOpts,
+                                StringRef FlagName);
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ASTUTILS_H

clang-tools-extra/trunk/clang-tidy/utils/ASTUtils.cpp

 //===---------- ASTUtils.cpp - clang-tidy ---------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ASTUtils.h"
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 using namespace ast_matchers;
 
 const FunctionDecl *getSurroundingFunction(ASTContext &Context,
                                            const Stmt &Statement) {
bool IsBinaryOrTernary(const Expr *E) {
   if (const auto *Operator =
           clang::dyn_cast<clang::CXXOperatorCallExpr>(E_base)) {
     return Operator->isInfixBinaryOp();
   }
 
   return false;
 }
 
+bool exprHasBitFlagWithSpelling(const Expr *Flags, const SourceManager &SM,
+                                const LangOptions &LangOpts,
+                                StringRef FlagName) {
+  // If the Flag is an integer constant, check it.
+  if (isa<IntegerLiteral>(Flags)) {
+    if (!SM.isMacroBodyExpansion(Flags->getLocStart()) &&
+        !SM.isMacroArgExpansion(Flags->getLocStart()))
+      return false;
+
+    // Get the marco name.
+    auto MacroName = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(Flags->getSourceRange()), SM, LangOpts);
+
+    return MacroName == FlagName;
+  }
+  // If it's a binary OR operation.
+  if (const auto *BO = dyn_cast<BinaryOperator>(Flags))
+    if (BO->getOpcode() == clang::BinaryOperatorKind::BO_Or)
+      return exprHasBitFlagWithSpelling(BO->getLHS()->IgnoreParenCasts(), SM,
+                                        LangOpts, FlagName) ||
+             exprHasBitFlagWithSpelling(BO->getRHS()->IgnoreParenCasts(), SM,
+                                        LangOpts, FlagName);
+
+  // Otherwise, assume it has the flag.
+  return true;
+}
+
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `android-cloexec-open
   Checks if the required file flag ``O_CLOEXEC`` exists in ``open()``,
   ``open64()`` and ``openat()``.
 
 - New `android-cloexec-fopen
   <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-fopen.html>`_ check
 
   Checks if the required mode ``e`` exists in the mode argument of ``fopen()``.
 
+- New `android-cloexec-socket
+  <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-socket.html>`_ check
+
+  Checks if the required file flag ``SOCK_CLOEXEC`` is present in the argument of
+  ``socket()``.
+
 - New `cert-dcl21-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl21-cpp.html>`_ check
 
   Checks if the overloaded postfix ``operator++/--`` returns a constant object.
 
 - New `cert-dcl58-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl58-cpp.html>`_ check
 

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

+.. title:: clang-tidy - android-cloexec-socket
+
+android-cloexec-socket
+======================
+
+``socket()`` should include ``SOCK_CLOEXEC`` in its type argument to avoid the
+file descriptor leakage. Without this flag, an opened sensitive file would
+remain open across a fork+exec to a lower-privileged SELinux domain.
+
+Examples:
+
+.. code-block:: c++
+
+  socket(domain, type, SOCK_STREAM);
+
+  // becomes
+
+  socket(domain, type, SOCK_STREAM | SOCK_CLOEXEC);

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

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    android-cloexec-creat
    android-cloexec-fopen
    android-cloexec-open
+   android-cloexec-socket
    boost-use-to-string
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl58-cpp
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-env33-c

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

+// RUN: %check_clang_tidy %s android-cloexec-socket %t
+
+#define SOCK_STREAM 1
+#define SOCK_DGRAM 2
+#define __O_CLOEXEC 3
+#define SOCK_CLOEXEC __O_CLOEXEC
+#define TEMP_FAILURE_RETRY(exp) \
+  ({                            \
+    int _rc;                    \
+    do {                        \
+      _rc = (exp);              \
+    } while (_rc == -1);        \
+  })
+
+extern "C" int socket(int domain, int type, int protocol);
+
+void a() {
+  socket(0, SOCK_STREAM, 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'socket' should use SOCK_CLOEXEC where possible [android-cloexec-socket]
+  // CHECK-FIXES: socket(0, SOCK_STREAM | SOCK_CLOEXEC, 0)
+  TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: 'socket'
+  // CHECK-FIXES: TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM | SOCK_CLOEXEC, 0))
+  socket(0, SOCK_STREAM | SOCK_DGRAM, 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'socket'
+  // CHECK-FIXES: socket(0, SOCK_STREAM | SOCK_DGRAM | SOCK_CLOEXEC, 0)
+  TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM | SOCK_DGRAM, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:56: warning: 'socket'
+  // CHECK-FIXES: TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM | SOCK_DGRAM | SOCK_CLOEXEC, 0))
+}
+
+void f() {
+  socket(0, 3, 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'socket'
+  // CHECK-FIXES: socket(0, 3 | SOCK_CLOEXEC, 0)
+  TEMP_FAILURE_RETRY(socket(0, 3, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: 'socket'
+  // CHECK-FIXES: TEMP_FAILURE_RETRY(socket(0, 3 | SOCK_CLOEXEC, 0))
+
+  int flag = 3;
+  socket(0, flag, 0);
+  TEMP_FAILURE_RETRY(socket(0, flag, 0));
+}
+
+namespace i {
+int socket(int domain, int type, int protocol);
+
+void d() {
+  socket(0, SOCK_STREAM, 0);
+  TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM, 0));
+  socket(0, SOCK_STREAM | SOCK_DGRAM, 0);
+  TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM | SOCK_DGRAM, 0));
+}
+
+} // namespace i
+
+void e() {
+  socket(0, SOCK_CLOEXEC, 0);
+  TEMP_FAILURE_RETRY(socket(0, SOCK_CLOEXEC, 0));
+  socket(0, SOCK_STREAM | SOCK_CLOEXEC, 0);
+  TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM | SOCK_CLOEXEC, 0));
+  socket(0, SOCK_STREAM | SOCK_CLOEXEC | SOCK_DGRAM, 0);
+  TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM | SOCK_CLOEXEC | SOCK_DGRAM, 0));
+}
+
+class G {
+public:
+  int socket(int domain, int type, int protocol);
+  void d() {
+    socket(0, SOCK_STREAM, 0);
+    TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM, 0));
+    socket(0, SOCK_STREAM | SOCK_DGRAM, 0);
+    TEMP_FAILURE_RETRY(socket(0, SOCK_STREAM | SOCK_DGRAM, 0));
+  }
+};