[clang-tidy] Check for sigaction in cert-sig30-c.

[balazske]

Summary

The checker recognizes handlers assigned to members of a
"struct sigaction" variable. If a function is assigned to
these members it is assumable that the function is used as signal handler
(function 'sigaction' is called with it).
Without this simplification the check is possible only in path-sensitive way.

[aaron.ballman]

clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp

91

You should be checking for the fully-qualified name so that this doesn't trip it up:

namespace awesome {
struct sigaction {
  const char *terrible = "haha";
};
}

92

I'd like to see a test case that this works properly with references in C++ (in addition to pointers).

clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp

152

I'd like to see a test case that shows this works if the sigaction object is within another structure. e.g.,

struct foo {
  struct sigaction act;
};

void test(void) {
  struct foo f;
  f.act.sa_handler = handler_sigaction5;
}

clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp

public:
   }
 
 private:
   CallbackType FindCallback;
 };
 } // namespace
 
 void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) {
-  const auto HandlerProtoType = functionProtoType(parameterCountIs(1));
-  const auto IsSignalFunction =
-      callee(functionDecl(hasName(SignalFun), parameterCountIs(2)));
-  const auto HandlerAsSecondArg = hasArgument(
-      1, declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")),
-                     unless(isExpandedFromMacro("SIG_IGN")),
-                     unless(isExpandedFromMacro("SIG_DFL")))
-             .bind("handler_expr"));
-  Finder->addMatcher(
-      callExpr(IsSignalFunction, HandlerAsSecondArg).bind("register_call"),
+  const auto HandlerExpr =
+      declRefExpr(hasDeclaration(
+                      functionDecl(parameterCountIs(1)).bind("handler_decl")),
+          .bind("handler_expr");
+  const auto SigactionHandlerExpr =
+      declRefExpr(hasDeclaration(
+                      functionDecl(parameterCountIs(3)).bind("handler_decl")))
+          .bind("handler_expr");
+
+  const auto IsSignalFunction =
+      callee(functionDecl(hasName(SignalFun), parameterCountIs(2)));
+
+  Finder->addMatcher(callExpr(IsSignalFunction, hasArgument(1, HandlerExpr))
+                         .bind("register_call"),
                      this);
+
+  auto StructSigactionObjectExpression =
+      hasObjectExpression(ignoringParenImpCasts(declRefExpr(
+          anyOf(hasType(recordDecl(hasName("sigaction"))),
+                hasType(pointsTo(recordDecl(hasName("sigaction"))))))));
+  auto HandlerMember = member(hasName("sa_handler"));
+  auto SigactionHandlerMember = member(hasName("sa_sigaction"));
+  auto HandlerAssign = binaryOperator(
+      isAssignmentOperator(),
+      hasLHS(memberExpr(StructSigactionObjectExpression, HandlerMember)),
+      hasRHS(ignoringParenImpCasts(HandlerExpr)));
+  auto SigactionHandlerAssign =
+      binaryOperator(isAssignmentOperator(),
+                     hasLHS(memberExpr(StructSigactionObjectExpression,
+                                       SigactionHandlerMember)),
+                     hasRHS(ignoringParenImpCasts(SigactionHandlerExpr)));
+  auto HandlerAssignInit =
+      varDecl(hasType(recordDecl(hasName("sigaction"))),
+              hasInitializer(initListExpr(
+                  hasInit(0, ignoringParenImpCasts(HandlerExpr)))));
+  auto SigactionHandlerAssignInit =
+      varDecl(hasType(recordDecl(hasName("sigaction"))),
+              hasInitializer(initListExpr(
+                  hasInit(1, ignoringParenImpCasts(SigactionHandlerExpr)))));
+
+  Finder->addMatcher(HandlerAssign.bind("handler_assign"), this);
+  Finder->addMatcher(HandlerAssignInit.bind("handler_assign"), this);
+  Finder->addMatcher(SigactionHandlerAssign.bind("handler_assign"), this);
+  Finder->addMatcher(SigactionHandlerAssignInit.bind("handler_assign"), this);
 }
 
 void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
+  const auto *AssignStmt = Result.Nodes.getNodeAs<Stmt>("handler_assign");
+  const auto *AssignDecl = Result.Nodes.getNodeAs<Decl>("handler_assign");
+  assert(!(SignalCall && (AssignStmt || AssignDecl)) &&
+         "A 'signal' call or assign to 'sigaction' member should be found but "
+         "not both together.");
   const auto *HandlerDecl =
       Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
   const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("handler_expr");
 
   // Visit each function encountered in the callgraph only once.
   llvm::DenseSet<const FunctionDecl *> SeenFunctions;
 
   // The worklist of the callgraph visitation algorithm.
while (!CalledFunctions.empty()) {
     if (isSystemCall(FunctionToCheck)) {
       if (isAllowedSystemCall(FunctionToCheck))
         continue;
 
       diag(FunctionCall->getBeginLoc(),
            "'%0' is considered as non asynchronous-safe and "
            "should not be called from a signal handler")
           << FunctionToCheck->getName();
+      diag(HandlerDecl->getBeginLoc(), "handler function declared here",
+           DiagnosticIDs::Note);
+      if (SignalCall)
         diag(SignalCall->getSourceRange().getBegin(),
              "signal handler registered here", DiagnosticIDs::Note);
-      diag(HandlerDecl->getBeginLoc(), "handler function declared here",
+      if (AssignStmt)
+        diag(AssignStmt->getSourceRange().getBegin(), "signal handler set here",
+             DiagnosticIDs::Note);
+      if (AssignDecl)
+        diag(AssignDecl->getSourceRange().getBegin(), "signal handler set here",
              DiagnosticIDs::Note);
       break;
     }
 
     // Get the body of the encountered non-system call function.
     const FunctionDecl *FunctionBody;
     if (!FunctionToCheck->hasBody(FunctionBody))
       continue;
 

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h

 
 #define SIGINT 1
 #define SIG_IGN _sig_ign
 #define SIG_DFL _sig_dfl
 
 typedef void (*sighandler_t)(int);
 sighandler_t signal(int signum, sighandler_t handler);
 
+typedef int siginfo_t;
+typedef int sigset_t;
+
+struct sigaction {
+  void (*sa_handler)(int);
+  void (*sa_sigaction)(int, siginfo_t *, void *);
+  sigset_t sa_mask;
+  int sa_flags;
+  void (*sa_restorer)(void);
+};
+
+int sigaction(int signum, const struct sigaction *act,
+              struct sigaction *oldact);
+
 #endif // _SIGNAL_H_

clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp

void test() {
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'other_call' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
 
   signal(SIGINT, SIG_IGN);
   signal(SIGINT, SIG_DFL);
 
   // Do not find problems here.
   signal(SIGINT, handler_bad, 1);
 }
+
+void handler_sigaction1(int) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void handler_sigaction2(int) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void handler_sigaction3(int) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void handler_sigaction4(int) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void sigaction_handler1(int, siginfo_t *, void *) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void sigaction_handler2(int, siginfo_t *, void *) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void sigaction_handler3(int, siginfo_t *, void *) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void sigaction_handler4(int, siginfo_t *, void *) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void test_sigaction1() {
+  struct sigaction SA;
+  // The checker can find only assignment of the handler function into the struct sigaction.
+  // It is assumed that the assigned function is (or at least may be) used as signal handler.
+  SA.sa_handler = handler_sigaction1;
+  SA.sa_handler = SIG_IGN;
+
+  struct sigaction SA1 = {handler_sigaction2, 0, 0, 0, 0};
+}
+
+void test_sigaction2() {
+  struct sigaction SA;
+  SA.sa_sigaction = sigaction_handler1;
+
+  struct sigaction SA1 = {0, sigaction_handler2, 0, 0, 0};
+}
+
+void test_sigaction3() {
+  struct sigaction SA;
+  SA.sa_handler = handler_sigaction3;
+  SA.sa_sigaction = sigaction_handler3;
+
+  struct sigaction SA1 = {handler_sigaction4, sigaction_handler4, 0, 0};
+}
+
+void handler_sigaction5(int) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void sigaction_handler5(int, siginfo_t *, void *) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void test_pointer(struct sigaction *SA) {
+  SA->sa_handler = handler_sigaction5;
+  SA->sa_sigaction = sigaction_handler5;
+}