[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

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

clang-tidy/android/CloexecSocketCheck.cpp

22

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

docs/ReleaseNotes.rst

79

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

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

6

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

21

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

93

nit: remove the empty line.

[alexfh]

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

21

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

s/presents/is present/

27

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

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

docs/ReleaseNotes.rst

79

s/presents/is present/

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

21

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.

66

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

[yawanng]

clang-tidy/utils/ASTUtils.h

27

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

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

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

28

FlagSpelling or FlagName could be better than just Flag.

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

6

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-tidy/android/AndroidTidyModule.cpp

 #include "CloexecCreatCheck.h"
 #include "CloexecFopenCheck.h"
 #include "CloexecOpenCheck.h"
+#include "CloexecSocketCheck.h"
 
 using namespace clang::ast_matchers;
 

     CheckFactories.registerCheck<CloexecCreatCheck>("android-cloexec-creat");
     CheckFactories.registerCheck<CloexecFopenCheck>("android-cloexec-fopen");
     CheckFactories.registerCheck<CloexecOpenCheck>("android-cloexec-open");
+    CheckFactories.registerCheck<CloexecSocketCheck>("android-cloexec-socket");
   }
 };
 

clang-tidy/android/CMakeLists.txt

   CloexecCreatCheck.cpp
   CloexecFopenCheck.cpp
   CloexecOpenCheck.cpp
+  CloexecSocketCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tidy/android/CloexecOpenCheck.cpp

 //===----------------------------------------------------------------------===//
 
 #include "CloexecOpenCheck.h"
+#include "../utils/ASTUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"

 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())));
 

 
   // 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 =

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-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-tidy/utils/ASTUtils.h

                                            const Stmt &Statement);
 // Determine whether Expr is a Binary or Ternary expression.
 bool IsBinaryOrTernary(const Expr *E);
+
+/// Check whether a macro flag is present in the given argument. Only consider
+/// 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, const char *Flag);
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tidy/utils/ASTUtils.cpp

 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
 
 namespace clang {
 namespace tidy {

   return false;
 }
 
+bool exprHasBitFlagWithSpelling(const Expr *Flags, const SourceManager &SM,
+                                const LangOptions &LangOpts, const char *Flag) {
+  // 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 == Flag;
+  }
+  // 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, Flag) ||
+             exprHasBitFlagWithSpelling(BO->getRHS()->IgnoreParenCasts(), SM,
+                                     LangOpts, Flag);
+
+  // Otherwise, assume it has the flag.
+  return true;
+}
+
 } // namespace utils
 } // namespace tidy
 } // namespace clang

docs/ReleaseNotes.rst

 
   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
 

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.
+
+Examples:
+
+.. code-block:: c++
+
+  socket(domain, type, SOCK_STREAM);
+
+  // becomes
+
+  socket(domain, type, SOCK_STREAM | SOCK_CLOEXEC);

docs/clang-tidy/checks/list.rst

    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

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));
+  }
+};