[clang-tidy] bugprone-signal-handler improvements: display call chain

[balazske]

Summary

Display notes for a possible call chain if an unsafe function is found to be
called (maybe indirectly) from a signal handler.
The call chain displayed this way includes probably not the first calls of
the functions, but it is a valid possible (in non path-sensitive way) one.

[aaron.ballman]

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

199–200

Do we have to worry about CallLevel + 1 being negative here? (Is there a need for an assert?)

207–208

Do we have to worry about call expressions for which we cannot find the declaration (like a call through a function pointer)? (Should we add some test coverage involving a call stack with a call through a function pointer?)

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h

39–43

Might as well not make this an optional parameter -- no real benefit from it (there's only two call sites).

[balazske]

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

199–200

Really yes, but here is no assert because it is on line 161 and the function is called only there. A df_iterator path contains the start and end node, and there should be at least one function if we have any report do show, the path length should be at least 1 (make assert for that?). (Later there could be cases when the function is called with path length 1.)

207–208

A declaration should always be there in the CallGraphNode, probably not a definition. The call graph does not insert nodes for function pointer calls.

[aaron.ballman]

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

199–200

Thanks for the explanation -- an assert that the path has at least 1 element wouldn't be awful, but also doesn't seem like something we'd get a lot of value from here. It's up to you if you want to add an assert or not.

207–208

Oh! I didn't know the CallGraph didn't model calls through function pointers (I had assumed we would model that because there's still a CallExpr involved.)

Can you add a test case involving a call stack with a call through a function pointer (with comments), just to demonstrate the behavior?

[njames93]

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

192–218

Small nit.

[balazske]

Added assert, added test with function pointer, improved text message generation.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h

 //===--- SignalHandlerCheck.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_BUGPRONE_SIGNALHANDLERCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H
 
 #include "../ClangTidyCheck.h"
 #include "clang/Analysis/CallGraph.h"
+#include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/StringSet.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 /// Checker for signal handler functions.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-signal-handler-check.html
 class SignalHandlerCheck : public ClangTidyCheck {
 public:
   enum class AsyncSafeFunctionSetType { Minimal, POSIX };
 
   SignalHandlerCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   bool isFunctionAsyncSafe(const FunctionDecl *FD) const;
   bool isSystemCallAsyncSafe(const FunctionDecl *FD) const;
   void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
-                 const CallExpr *SignalCall, const FunctionDecl *HandlerDecl);
+                 bool DirectHandler);
+  void reportHandlerCommon(llvm::df_iterator<clang::CallGraphNode *> Itr,
+                           const CallExpr *SignalCall,
+                           const FunctionDecl *HandlerDecl,
+                           const Expr *HandlerRef);
 
-  CallGraph CG;
+  clang::CallGraph CG;
 
   AsyncSafeFunctionSetType AsyncSafeFunctionSet;
   llvm::StringSet<> &ConformingFunctions;
 
   static llvm::StringSet<> MinimalConformingFunctions;
   static llvm::StringSet<> POSIXConformingFunctions;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H

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

CG.addToCallGraph(const_cast<TranslationUnitDecl *>(
         HandlerDecl->getTranslationUnitDecl()));
     assert(CG.size() > 1 &&
            "There should be at least one function added to call graph.");
   }
 
   // Check for special case when the signal handler itself is an unsafe external
   // function.
   if (!isFunctionAsyncSafe(HandlerDecl)) {
-    reportBug(HandlerDecl, HandlerExpr, SignalCall, HandlerDecl);
+    reportBug(HandlerDecl, HandlerExpr, /*DirectHandler=*/true);
     return;
   }
 
   CallGraphNode *HandlerNode = CG.getNode(HandlerDecl);
   // Signal handler can be external but not unsafe, no call graph in this case.
   if (!HandlerNode)
     return;
   // Start from signal handler and visit every function call.
   for (auto Itr = llvm::df_begin(HandlerNode), ItrE = llvm::df_end(HandlerNode);
        Itr != ItrE; ++Itr) {
     const auto *CallF = dyn_cast<FunctionDecl>((*Itr)->getDecl());
     if (CallF && !isFunctionAsyncSafe(CallF)) {
       assert(Itr.getPathLength() >= 2);
       reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr),
-                SignalCall, HandlerDecl);
+                /*DirectHandler=*/false);
+      reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr);
     }
   }
 }
 
 bool SignalHandlerCheck::isFunctionAsyncSafe(const FunctionDecl *FD) const {
   if (isSystemCall(FD))
     return isSystemCallAsyncSafe(FD);
   // For external (not checkable) functions assume that these are unsafe.
bool SignalHandlerCheck::isSystemCallAsyncSafe(const FunctionDecl *FD) const {
   // FIXME: Improve for C++ (check for namespace).
   if (ConformingFunctions.count(II->getName()))
     return true;
 
   return false;
 }
 
 void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
-                                   const Expr *CallOrRef,
-                                   const CallExpr *SignalCall,
-                                   const FunctionDecl *HandlerDecl) {
+                                   const Expr *CallOrRef, bool DirectHandler) {
   diag(CallOrRef->getBeginLoc(),
-       "%0 may not be asynchronous-safe; "
-       "calling it from a signal handler may be dangerous")
-      << CalledFunction;
-  diag(SignalCall->getSourceRange().getBegin(),
-       "signal handler registered here", DiagnosticIDs::Note);
-  diag(HandlerDecl->getBeginLoc(), "handler function declared here",
-       DiagnosticIDs::Note);
+       "%0 may not be asynchronous-safe; %select{calling it from|using it as}1 "
+       "a signal handler may be dangerous")
+      << CalledFunction << DirectHandler;
+}
+
+void SignalHandlerCheck::reportHandlerCommon(
+    llvm::df_iterator<clang::CallGraphNode *> Itr, const CallExpr *SignalCall,
+    const FunctionDecl *HandlerDecl, const Expr *HandlerRef) {
+  int CallLevel = Itr.getPathLength() - 2;
+  assert(CallLevel >= -1 && "Empty iterator?");
+
+  const CallGraphNode *Caller = Itr.getPath(CallLevel + 1), *Callee = nullptr;
+  while (CallLevel >= 0) {
+    Callee = Caller;
+    Caller = Itr.getPath(CallLevel);
+    const Expr *CE = findCallExpr(Caller, Callee);
+    diag(CE->getBeginLoc(), "function %0 called here from %1",
+         DiagnosticIDs::Note)
+        << cast<FunctionDecl>(Callee->getDecl())
+        << cast<FunctionDecl>(Caller->getDecl());
+    --CallLevel;
+  }
+
+  diag(HandlerRef->getBeginLoc(),
+       "function %0 registered here as signal handler", DiagnosticIDs::Note)
+      << HandlerDecl;
 }
 
 // This is the minimal set of safe functions.
 // https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
 llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
     "signal", "abort", "_Exit", "quick_exit"};
 
 // The POSIX-defined set of safe functions.
 // https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c

 // RUN: %check_clang_tidy %s bugprone-signal-handler %t -- -- -isystem %S/Inputs/Headers
 
 #include "signal.h"
 #include "stdlib.h"
 #include "stdio.h"
 #include "system-other.h"
 
 // The function should be classified as system call even if there is
 // declaration the in source file.
 // FIXME: The detection works only if the first declaration is in system
 // header.
 int printf(const char *, ...);
 typedef void (*sighandler_t)(int);
 sighandler_t signal(int signum, sighandler_t handler);
 
-void handler_abort(int) {
-  abort();
-}
+void f_extern();
 
-void handler_other(int) {
+void handler_printf(int) {
   printf("1234");
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_printf'
+  // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_printf' registered here as signal handler
 }
 
-void handler_signal(int) {
-  // FIXME: It is only OK to call signal with the current signal number.
-  signal(0, SIG_DFL);
+void test_printf() {
+  signal(SIGINT, handler_printf);
 }
 
-void f_ok() {
-  abort();
+void handler_extern(int) {
+  f_extern();
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'handler_extern'
+  // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_extern' registered here as signal handler
 }
 
-void f_bad() {
-  printf("1234");
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+void test_extern() {
+  signal(SIGINT, handler_extern);
 }
 
-void f_extern();
+void f_ok() {
+  abort();
+}
 
 void handler_ok(int) {
   f_ok();
 }
 
+void test_ok() {
+  signal(SIGINT, handler_ok);
+}
+
+void f_bad() {
+  printf("1234");
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_bad'
+  // CHECK-NOTES: :[[@LINE+5]]:3: note: function 'f_bad' called here from 'handler_bad'
+  // CHECK-NOTES: :[[@LINE+8]]:18: note: function 'handler_bad' registered here as signal handler
+}
+
 void handler_bad(int) {
   f_bad();
 }
 
-void handler_extern(int) {
-  f_extern();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+void test_bad() {
+  signal(SIGINT, handler_bad);
+}
+
+void f_bad1() {
+  printf("1234");
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_bad1'
+  // CHECK-NOTES: :[[@LINE+6]]:3: note: function 'f_bad1' called here from 'f_bad2'
+  // CHECK-NOTES: :[[@LINE+9]]:3: note: function 'f_bad2' called here from 'handler_bad1'
+  // CHECK-NOTES: :[[@LINE+13]]:18: note: function 'handler_bad1' registered here as signal handler
+}
+
+void f_bad2() {
+  f_bad1();
+}
+
+void handler_bad1(int) {
+  f_bad2();
+  f_bad1();
+}
+
+void test_bad1() {
+  signal(SIGINT, handler_bad1);
 }
 
-void test_false_condition(int) {
+void handler_abort(int) {
+  abort();
+}
+
+void handler_signal(int) {
+  // FIXME: It is only OK to call signal with the current signal number.
+  signal(0, SIG_DFL);
+}
+
+void handler_false_condition(int) {
   if (0)
     printf("1234");
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:5: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:5: note: function 'printf' called here from 'handler_false_condition'
+  // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_false_condition' registered here as signal handler
+}
+
+void test_false_condition() {
+  signal(SIGINT, handler_false_condition);
 }
 
-void test_multiple_calls(int) {
+void handler_multiple_calls(int) {
   f_extern();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'handler_multiple_calls'
+  // CHECK-NOTES: :[[@LINE+10]]:18: note: function 'handler_multiple_calls' registered here as signal handler
   printf("1234");
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_multiple_calls'
+  // CHECK-NOTES: :[[@LINE+6]]:18: note: function 'handler_multiple_calls' registered here as signal handler
   f_extern();
   // first 'f_extern' call found only
 }
 
+void test_multiple_calls() {
+  signal(SIGINT, handler_multiple_calls);
+}
+
 void f_recursive();
 
-void test_recursive(int) {
+void handler_recursive(int) {
   f_recursive();
   printf("");
   // first 'printf' call (in other function) found only
 }
 
 void f_recursive() {
   f_extern();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'f_recursive'
+  // CHECK-NOTES: :[[@LINE-9]]:3: note: function 'f_recursive' called here from 'handler_recursive'
+  // CHECK-NOTES: :[[@LINE+10]]:18: note: function 'handler_recursive' registered here as signal handler
+  printf("");
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_recursive'
+  // CHECK-NOTES: :[[@LINE-14]]:3: note: function 'f_recursive' called here from 'handler_recursive'
+  // CHECK-NOTES: :[[@LINE+5]]:18: note: function 'handler_recursive' registered here as signal handler
+  handler_recursive(2);
+}
+
+void test_recursive() {
+  signal(SIGINT, handler_recursive);
+}
+
+void f_multiple_paths() {
   printf("");
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
-  f_recursive(2);
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_multiple_paths'
+  // CHECK-NOTES: :[[@LINE+5]]:3: note: function 'f_multiple_paths' called here from 'handler_multiple_paths'
+  // CHECK-NOTES: :[[@LINE+9]]:18: note: function 'handler_multiple_paths' registered here as signal handler
 }
 
-void test() {
+void handler_multiple_paths(int) {
+  f_multiple_paths();
+  f_multiple_paths();
+}
+
+void test_multiple_paths() {
+  signal(SIGINT, handler_multiple_paths);
+}
+
+void handler_function_pointer(int) {
+  void (*fp)() = f_extern;
+  // Call with function pointer is not evalauted by the check.
+  (*fp)();
+}
+
+void test_function_pointer() {
+  signal(SIGINT, handler_function_pointer);
+}
+
+void test_other() {
   signal(SIGINT, handler_abort);
   signal(SIGINT, handler_signal);
-  signal(SIGINT, handler_other);
-
-  signal(SIGINT, handler_ok);
-  signal(SIGINT, handler_bad);
-  signal(SIGINT, handler_extern);
 
   signal(SIGINT, _Exit);
   signal(SIGINT, other_call);
-  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:18: warning: 'other_call' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
+  signal(SIGINT, f_extern);
+  // CHECK-NOTES: :[[@LINE-1]]:18: warning: 'f_extern' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
 
   signal(SIGINT, SIG_IGN);
   signal(SIGINT, SIG_DFL);
-
-  signal(SIGINT, test_false_condition);
-  signal(SIGINT, test_multiple_calls);
-  signal(SIGINT, test_recursive);
 }