[clang-tidy] Add new check for SEI CERT rule SIG30-C

[balazske]

Summary

SIG30-C. Call only asynchronous-safe functions within signal handlers

First version of this check, only minimal list of functions is allowed
("strictly conforming" case). Later a checker option can be added
to support the POSIX list of allowed functions (taken from the rule's
description).

[Eugene.Zelenko]

Please add entry in Release Notes.

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

26 ↗

(On Diff #290949)

Only type definitions should be in anonymous namespace. Everything else must be static.

142 ↗

(On Diff #290949)

Please add newline.

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

35 ↗

(On Diff #290949)

Please add newline.

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

7

Please remove This check.

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

23

A lot of unrelated changes.

[balazske]

• Code formatting fixes.
• Updated description.
• Improved CalledFunctionsCollector.

[balazske]

It looks like that the clang-tidy/add_new_check.py script does not work correctly, at least it "corrupts" the list.rst file, and creates files with no newline at end.

[Eugene.Zelenko]

Release Notes were not updated yet.

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

11

Please make all strings not longer then 80 symbols.

[balazske]

Added entry to release notes and fixed wrong comment in test headers.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

143

Please rebase from trunk. New checks section is above and somehow header is missed in your file.

[balazske]

Changed checker messages, reformatted text, rebase.

[balazske]

Ping

[baloghadamsoftware]

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

35 ↗

(On Diff #291524)

The assumption is basically right, we do not repeat declarations from system headers but maybe we could loop over the redeclaration chain.

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

2–18

Please add at least one minimal code example. (E.g. from the tests.)

[baloghadamsoftware]

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

31 ↗

(On Diff #291524)

Why? In C++ we have everything in std namespace, such as std::signal(), std::abort() or std::_Exit(). In C++ the general rule is to use them instead of the global C variants.

40 ↗

(On Diff #291524)

The name suggests that this function checks for both system call and allowed call. I would either rename this function to simply isAllowedCall() or at least put an assertion to the beginning: assert(isSystemCall(FD));.

44 ↗

(On Diff #291524)

Maybe you could use IdentifierInfo instead of string comparisons.

81 ↗

(On Diff #291524)

More readable would be this way:

const auto HandlerExpr = declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")),
   unless(isExpandedFromMacro("SIG_IGN")),
   unless(isExpandedFromMacro("SIG_DFL")))
      .bind("handler_expr");
Finder->addMatcher(
   callExpr(IsSignalFunction, hasArgument(1, HandlerExpr)).bind("register_call"),
   this);

95 ↗

(On Diff #291524)

Do we really need to store FunctionDecl in the map? The whole code would be much simpler if you only store the call expression and the retrieve the callee declaration once at the beginning of the loop body. Beside simplicity this would also reduce the memory footprint and surely not increase the execution time.

[aaron.ballman]

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

31–33 ↗

(On Diff #291524)

This seems incorrect to me. ::std::quick_exit() resolves to ::quick_exit(), so why should that call be ignored as a system call? I would assume that anything in namespace std is a system call. It's a bit questionable whether a user-written template specialization in namespace std should be handled that way, but I think that's still reasonable to consider as a system call.

34 ↗

(On Diff #291524)

re-declaration -> redeclaration

41 ↗

(On Diff #291524)

A function without an identifier is not a system call, so I would have expected this to return false based on the function name.

43 ↗

(On Diff #291524)

We don't typically use top-level const in the project, so this can be dropped. Same comment applies elsewhere in the patch.

44–46 ↗

(On Diff #291524)

The logic isn't quite correct here as this will claim to be an allowed system call:

namespace awesome {
  void quick_exit(void); // Considers this to be a system call
}

You should be checking the namespace as well.

Also, this list is very incomplete depending on your platform. The CERT rule lists a whole bunch of POSIX functions that are required to be async signal safe. I am guessing Microsoft likely has a list somewhere as well. I worry about the number of false positives this check will issue if we don't at least consider POSIX (where signals are much, much more useful than in strictly conforming C).

73 ↗

(On Diff #291524)

Similar issue here with finding functions in the wrong namespace.

117–118 ↗

(On Diff #291524)

How about: '%0' may not be asynchronous-safe; calling it from a signal handler may be dangerous?

135 ↗

(On Diff #291524)

emplace_back(FD, CE)?

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

14

I would document this as: Any function that cannot be determined to be an asynchronous-safe function call is assumed to be non-asynchronous-safe by the checker, including function calls for which only the declaration of the called function is visible.

[balazske]

Added support for C++ code.
Improved detection of 'signal' function.
Simplified collection of called functions.

[balazske]

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

95 ↗

(On Diff #291524)

The code was changed to store only the CallExpr. The first item is not a function call but the reference to function in the signal call, so it needs special handling.

[aaron.ballman]

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

33 ↗

(On Diff #295538)

I'm not certain I understand why we're looking through the entire redeclaration chain to see if the function is ever mentioned in a system header. I was expecting we'd look at the expansion location of the declaration and see if that's in a system header, which is already handled by the isExpansionInSystemHeader() matcher. Similar below.

84 ↗

(On Diff #295538)

I think this can be simplified to remove the anyOf:

functionDecl(hasAnyName("::signal", "::std::signal"), isExpansionInSystemHeader())

should be sufficient (if there's a function named signal in a system header that has the wrong parameter count, I'd be surprised).

97 ↗

(On Diff #295538)

All of these should be const auto * (wow, the lint pre-merge check was actually useful for once!)

165 ↗

(On Diff #295538)

How about: Unnamed functions are explicitly not allowed.

171 ↗

(On Diff #295538)

I think a configuration option is needed for users to be able to add their own conforming functions and by default, that list should include the functions that POSIX specifies as being async signal safe (at least on POSIX systems, similar for Windows if Microsoft documents such a list).

[balazske]

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

33 ↗

(On Diff #295538)

This function is called from SignalHandlerCheck::check when any function call is found. So the check for system header is needed. It was unclear to me what the "expansion location" means but it seems to work if using that expansion location and checking for system header, instead of this loop. I will update the code.

84 ↗

(On Diff #295538)

I was not aware of the possibility of using namespaces in the name, it is really more simple this way (and the isExpansionInSystemHeader seems to work good here).

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

14

"including function calls for which only the declaration of the called function is visible": Is this the better approach? The checker does not make warning for such functions in the current state.

[balazske]

I plan to add the option for extended set of asynchronous-safe functions (defined by the POSIX list) in a next patch. There is other possible improvement to check at least something of the criteria listed here for C++17 .

[aaron.ballman]

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

33 ↗

(On Diff #295538)

This function is called from SignalHandlerCheck::check when any function call is found. So the check for system header is needed.

The check for the system header isn't what I was concerned by, it was the fact that we're walking the entire redeclaration chain to see if *any* declaration is in a system header that I don't understand the purpose of.

It was unclear to me what the "expansion location" means but it seems to work if using that expansion location and checking for system header, instead of this loop. I will update the code.

The spelling location is where the user wrote the code and the expansion location is where the macro name is written, but thinking on it harder, that shouldn't matter for this situation as either location would be in the system header anyway.

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

14

Ooh, thank you for calling this out, you're right that I wasn't describing the current behavior.

My thinking is: most system functions aren't safe to call within a signal handler and user-defined functions will eventually call a system function more often than they won't, so assuming a function for which you can't see the definition is not signal safe is a somewhat reasonable assumption, but may have false positives. However, under the assumption that most signal handlers are working as intended, then perhaps it's better to assume that the author of such unseen function bodies did the right thing as you're doing, but then you may have false negatives.

Given that the CERT rules are about security, I think it's better to err on the side of more false positives than more false negatives, but it's open for debate.

[balazske]

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

33 ↗

(On Diff #295538)

The function declaration is not often a macro name so there is no "expansion location" or the same as the original location. My concern was that if there is a declaration of system call function in a source file (like void abort(void); in .c file) for any reason, we may find this declaration instead of the one in the header file, if not looping over the declaration chain.

[aaron.ballman]

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

33 ↗

(On Diff #295538)

The function declaration is not often a macro name so there is no "expansion location" or the same as the original location.

Agreed.

My concern was that if there is a declaration of system call function in a source file (like void abort(void); in .c file) for any reason, we may find this declaration instead of the one in the header file, if not looping over the declaration chain.

Typically, when a C user does something like that, it's because they're explicitly *not* including the header file at all (they're just forward declaring the function so they can use it) and so we'd get the logic wrong for them anyway because we'd never find the declaration in the system header.

Using the canonical declaration is more typical and would realistically fail only in circumstances like forward declaring a system header function and then later including the system header itself. That said, I suppose your approach is defensible enough. Redeclaration chains are hopefully short enough that it isn't a performance hit.

[balazske]

Updated check for system function.
Updated documentation.
Added more test cases.

[balazske]

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

33 ↗

(On Diff #295538)

I changed back to the original code to search the entire redeclaration chain. Otherwise it can be fooled with declarations before or after inclusion of the system header. Such declarations were added to the test file (it did not pass if isExpansionInSystemHeader was used).

41 ↗

(On Diff #291524)

I would think that if the function is an operation on a std object (std::vector) it should be classified as system call, and these operations (or many of them) look not asynchronous-safe.

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

14

Changed the behavior to report external (user) functions as non-asynchronous-safe. This is the more safe option, and consistent with "only the explicitly allowed functions are safe to call".

[aaron.ballman]

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

27 ↗

(On Diff #295840)

return llvm::any_of(FD->redecls(), [](const FunctionDecl *D) {
  return D->getASTContext().getSourceManager().isInSystemHeader(D->getLocation());
});

117–118 ↗

(On Diff #295840)

For correctness, I think you need to handle more than just calls to function declarations -- for instance, this should be just as problematic:

void some_signal_handler(int sig) {
  []{ puts("this should not be an escape hatch for the check); }();
}

even though the call expression in the signal handler doesn't resolve back to a function declaration. (Similar for blocks instead of lambdas.) WDYT?

134 ↗

(On Diff #295840)

const auto *

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

52 ↗

(On Diff #295840)

I'd also like to see a test case where the handler to signal call is itself not a function call:

std::signal(SIGINT, [](int sig) {
  puts("I did the bad thing this way"); // should be diagnosed, yes?
});

[aaron.ballman]

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

41 ↗

(On Diff #291524)

Hmm, that's an interesting point I hadn't considered and I don't know what the correct answer is as it relates to this check. For instance, this code is bad, but not because of sig30-C:

std::vector<int> some_global_vector;
void sig_handler(int sig) {
  int &i = some_global_vector[0];
  ...
}

I doubt that operator[]() is actually making any system calls under the hood, so it's fine per sig30-c, but the code is still bad (it should fail sig31-c about not using shared objects from signals). On the flip side:

std::packaged_task<void(int)> some_task;
void sig_handler(int sig) {
  some_task(sig); // Who knows what this will execute when it calls operator()()
}

[balazske]

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

117–118 ↗

(On Diff #295840)

I do not know how many other cases could be there. Probably this can be left for future improvement, the checker is mainly usable for C code then. There is a clang::CallGraph functionality that could be used instead of FunctionCallCollector but the CallExpr for the calls is not provided by it so it does not work for this case. Maybe there is other similar functionality that is usable?

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

52 ↗

(On Diff #295840)

This is again a new case to handle. A new matcher must be added to detect this. But I am not sure how many other cases are there, or is it worth to handle all of them.

[aaron.ballman]

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

117–118 ↗

(On Diff #295840)

Given that we want it in the CERT module, we should try to ensure it follows the rule as closely as we can. I went and checked what the C++ rules say about this and... it was interesting to notice that SIG30-C is not one of the C rules included by reference in C++ (https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336).

It's not clear to me that this rule was accidentally tagged as not-for-cpp or not, so I'd say it's fine to ignore lambdas for the moment but we may have some follow-up work if CERT changes the rule to be included in C++. My recommendation is: make the check a C-only check for now, document it as such, and I'll ping the folks at CERT to see if this rule was mistagged or not. WDYT?

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

52 ↗

(On Diff #295840)

Let's punt on this until we hear back from CERT on whether this rule should be supported in C++.

[aaron.ballman]

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

117–118 ↗

(On Diff #295840)

Ah, this rule really is a C-only rule, because https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function is the C++ rule. So I think the SIG30-C checker should be run in C-only mode and we can ignore the C++isms in it.

FWIW, we have an ongoing discussion about MSC54-CPP in https://reviews.llvm.org/D33825.

[balazske]

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

117–118 ↗

(On Diff #295840)

Probably this checker can be merged with the other in D33825. According to cppreference (https://en.cppreference.com/w/cpp/utility/program/signal) the check for the called functions should be present for C++ too. And the other checker should do a similar lookup of called functions as this checker, including lambdas and C++ specific things.

[aaron.ballman]

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

117–118 ↗

(On Diff #295840)

While you would think that, it's a bit more complicated unfortunately. The C++ committee has been moving forward with this paper http://wg21.link/p0270 so that C++ is no longer tied to the same constraints as C. That may suggest that separate checks are appropriate, or it may still mean we want to merge the checks into one.

[balazske]

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

117–118 ↗

(On Diff #295840)

I think it is more convenient to merge the two checkers. The visitation of called functions goes the same way, the support for C++ constructs should not cause problems if used with C code. The handling of a detected function can be different code for C and C++ mode but if there are similar parts code can be reused.
Otherwise code of this checker would be a better starting point for "SignalHandlerMustBePlainOldFunctionCheck" because it handles detection of the signal function already better specially for C++.

[aaron.ballman]

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

117–118 ↗

(On Diff #295840)

Okay, I could see that. Would you like to collaborate with the author of D33825 to see if you can produce a combined check? Or would you prefer to wait for that review to land for C++ and then modify it for C? (Or some other approach entirely?)

[balazske]

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

117–118 ↗

(On Diff #295840)

For me it looks better to pull in code from the other review. I found multiple issues with it but the detection code is usable here. It should be better however to first commit a simple version of the checker, for C functions only, and extend it in smaller patches.

[balazske]

Removed C++ support.
Other small changes.

[aaron.ballman]

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

33 ↗

(On Diff #295538)

I don't think that's necessary (we aren't consistent about whether we check the entire redecl chain, so I worry about inconsistent behavior between checks). If you are looking at the canonical declaration (instead of just any declaration), you'll always be looking at the *first* declaration encountered, which is generally sufficient.

// a.c
#include <sysheader.h> // Most common case will be inclusion directly from a header, this works fine

// b.c
extern void sysfunc(void); // Next most common case will be extern declaration, can't catch this with either approach.

// c.c
#include <sysheader.h> // Canonical declaration, so this works
extern void sysfunc(void); // redecl won't matter

// d.c
extern void sysfunc(void); // Canonical declaration, so this fails
#include <sysheader.h> // But at the same time, who does this?

I don't insist on a change, but as a mental note to myself and other authors, we should probably try to have a more consistent policy here.

131 ↗

(On Diff #297828)

Remove commented-out code?

162 ↗

(On Diff #297828)

You may want a FIXME here that this will eventually be wrong in C++ mode. Consider declarations like:

namespace foo {
void abort();

inline namespace bar {
void quick_exit(int);
}
}

namespace {
void signal();
}

If we form a FunctionDecl to any of those functions, we shouldn't count those as the conforming functions.

175 ↗

(On Diff #297828)

You can pass CalledFunction directly -- any NamedDecl gets automatically formatted by the diagnostics engine. You should also drop the single quotes around %0 as they'll be automatically added by the diagnostics engine.

clang-tools-extra/docs/ReleaseNotes.rst

146

Can you mention that this check is currently for C only?

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

2–18

Also, please document that the check is currently limited to C code.

clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cert-sig30-c_cpp.h

1 ↗

(On Diff #297828)

I think this file can be removed entirely for now.

[balazske]

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

33 ↗

(On Diff #295538)

If I understand correctly, the problem is that other checkers already test for "system calls" by checking only the canonical declaration (or "isExpansionInSystemHeader"). It would be better to have a common function or AST matcher for this purpose and update all checkers to use that (even in clang static analyzer).

[balazske]

Handled review comments.

[balazske]

I think the name of this checker should be changed. It could in future not only check for the SIG30-C rule. (Plan is to include C++ checks too, and SIG31-C could be checked in this checker too.) It can be called "bugprone-signal-handler" instead?

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

33 ↗

(On Diff #295538)

My decision would be to use the current code, it is more safe. There is not a documented guarantee (?) that canonical declaration is always the first one, and the last "d.c" case may happen:

// d.h
extern void sysfunc(void); // used by some construct in this file or just forgotten here

// d.c
#include "d.h"
#include <sysheader.h>

[aaron.ballman]

In D87449#2358579, @balazske wrote:

I think the name of this checker should be changed. It could in future not only check for the SIG30-C rule. (Plan is to include C++ checks too, and SIG31-C could be checked in this checker too.) It can be called "bugprone-signal-handler" instead?

I have no issues putting this into the bugprone module and then aliasing to it from the cert module.

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

33 ↗

(On Diff #295538)

There is not a documented guarantee (?) that canonical declaration is always the first one, and the last "d.c" case may happen:

Hmm, I am pretty sure this is our documentation on the topic: https://github.com/llvm/llvm-project/blob/ad8e900cb3c60873e954221816aafc6767222de2/clang/include/clang/AST/Redeclarable.h#L30 While it says "often", I'm not aware of a circumstance where that doesn't hold true.

As for the case you pointed out, yes, that's possible. However, to my original point, that's more consistent with the way clang-tidy checks currently work than what you're doing, which is why I've been pushing back on it. It's not a particularly worrisome case, but having inconsistent behavior between checks makes all checks harder for users to reason about because they have to remember which decision was made for any given check.

[balazske]

Rename of the checker.
Using canonical decl for system function detection.

[aaron.ballman]

I think this LGTM aside from a few minor nits. Thank you for working on this!

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

13–15

I think this bit about the CERT link should move into the main checker documentation (if only because the alias documentation will redirect to the main documentation after five seconds).

16

Same for the C-only nature of the check.

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

21

Might as well drop the parameter names here as well.

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

14–15

Might as well drop the parameter names here too.

[balazske]

Updated according to comments.

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

 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "NoEscapeCheck.h"
 #include "NotNullTerminatedResultCheck.h"
 #include "ParentVirtualCallCheck.h"
 #include "PosixReturnCheck.h"
 #include "RedundantBranchConditionCheck.h"
 #include "ReservedIdentifierCheck.h"
+#include "SignalHandlerCheck.h"
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "SpuriouslyWakeUpFunctionsCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
         "bugprone-not-null-terminated-result");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
     CheckFactories.registerCheck<PosixReturnCheck>(
         "bugprone-posix-return");
     CheckFactories.registerCheck<ReservedIdentifierCheck>(
         "bugprone-reserved-identifier");
+    CheckFactories.registerCheck<SignalHandlerCheck>("bugprone-signal-handler");
     CheckFactories.registerCheck<SignedCharMisuseCheck>(
         "bugprone-signed-char-misuse");
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
     CheckFactories.registerCheck<SpuriouslyWakeUpFunctionsCheck>(
         "bugprone-spuriously-wake-up-functions");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   NoEscapeCheck.cpp
   NotNullTerminatedResultCheck.cpp
   ParentVirtualCallCheck.cpp
   PosixReturnCheck.cpp
   RedundantBranchConditionCheck.cpp
   ReservedIdentifierCheck.cpp
+  SignalHandlerCheck.cpp
   SignedCharMisuseCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   SpuriouslyWakeUpFunctionsCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousEnumUsageCheck.cpp

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 "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:
+  SignalHandlerCheck(StringRef Name, ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
+                 const CallExpr *SignalCall, const FunctionDecl *HandlerDecl);
+  bool isSystemCallAllowed(const FunctionDecl *FD) const;
+
+  static llvm::StringSet<> StrictConformingFunctions;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H

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

+//===--- SignalHandlerCheck.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 "SignalHandlerCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/CallGraph.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include <iterator>
+#include <queue>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+static bool isSystemCall(const FunctionDecl *FD) {
+  // Find a possible redeclaration in system header.
+  // FIXME: Looking at the canonical declaration is not the most exact way
+  // to do this.
+
+  // Most common case will be inclusion directly from a header.
+  // This works fine by using canonical declaration.
+  // a.c
+  // #include <sysheader.h>
+
+  // Next most common case will be extern declaration.
+  // Can't catch this with either approach.
+  // b.c
+  // extern void sysfunc(void);
+
+  // Canonical declaration is the first found declaration, so this works.
+  // c.c
+  // #include <sysheader.h>
+  // extern void sysfunc(void); // redecl won't matter
+
+  // This does not work with canonical declaration.
+  // Probably this is not a frequently used case but may happen (the first
+  // declaration can be in a non-system header for example).
+  // d.c
+  // extern void sysfunc(void); // Canonical declaration, not in system header.
+  // #include <sysheader.h>
+
+  return FD->getASTContext().getSourceManager().isInSystemHeader(
+      FD->getCanonicalDecl()->getLocation());
+}
+
+AST_MATCHER(FunctionDecl, isSystemCall) { return isSystemCall(&Node); }
+
+// This is the  minimal set of safe functions.
+// FIXME: Add checker option to allow a POSIX compliant extended set.
+llvm::StringSet<> SignalHandlerCheck::StrictConformingFunctions{
+    "signal", "abort", "_Exit", "quick_exit"};
+
+SignalHandlerCheck::SignalHandlerCheck(StringRef Name,
+                                       ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+
+bool SignalHandlerCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  // FIXME: Make the checker useful on C++ code.
+  if (LangOpts.CPlusPlus)
+    return false;
+
+  return true;
+}
+
+void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  auto SignalFunction = functionDecl(hasAnyName("::signal", "::std::signal"),
+                                     parameterCountIs(2), isSystemCall());
+  auto HandlerExpr =
+      declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")),
+                  unless(isExpandedFromMacro("SIG_IGN")),
+                  unless(isExpandedFromMacro("SIG_DFL")))
+          .bind("handler_expr");
+  Finder->addMatcher(
+      callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
+          .bind("register_call"),
+      this);
+}
+
+void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
+  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.
+  std::deque<const CallExpr *> CalledFunctions;
+
+  auto ProcessFunction = [&](const FunctionDecl *F, const Expr *CallOrRef) {
+    // Ensure that canonical declaration is used.
+    F = F->getCanonicalDecl();
+
+    // Do not visit function if already encountered.
+    if (!SeenFunctions.insert(F).second)
+      return true;
+
+    // Check if the call is allowed.
+    // Non-system calls are not considered.
+    if (isSystemCall(F)) {
+      if (isSystemCallAllowed(F))
+        return true;
+
+      reportBug(F, CallOrRef, SignalCall, HandlerDecl);
+
+      return false;
+    }
+
+    // Get the body of the encountered non-system call function.
+    const FunctionDecl *FBody;
+    if (!F->hasBody(FBody)) {
+      reportBug(F, CallOrRef, SignalCall, HandlerDecl);
+      return false;
+    }
+
+    // Collect all called functions.
+    auto Matches = match(decl(forEachDescendant(callExpr().bind("call"))),
+                         *FBody, FBody->getASTContext());
+    for (const auto &Match : Matches) {
+      const auto *CE = Match.getNodeAs<CallExpr>("call");
+      if (isa<FunctionDecl>(CE->getCalleeDecl()))
+        CalledFunctions.push_back(CE);
+    }
+
+    return true;
+  };
+
+  if (!ProcessFunction(HandlerDecl, HandlerExpr))
+    return;
+
+  // Visit the definition of every function referenced by the handler function.
+  // Check for allowed function calls.
+  while (!CalledFunctions.empty()) {
+    const CallExpr *FunctionCall = CalledFunctions.front();
+    CalledFunctions.pop_front();
+    // At insertion we have already ensured that only function calls are there.
+    const auto *F = cast<FunctionDecl>(FunctionCall->getCalleeDecl());
+
+    if (!ProcessFunction(F, FunctionCall))
+      break;
+  }
+}
+
+bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const {
+  const IdentifierInfo *II = FD->getIdentifier();
+  // Unnamed functions are not explicitly allowed.
+  if (!II)
+    return false;
+
+  // FIXME: Improve for C++ (check for namespace).
+  if (StrictConformingFunctions.count(II->getName()))
+    return true;
+
+  return false;
+}
+
+void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
+                                   const Expr *CallOrRef,
+                                   const CallExpr *SignalCall,
+                                   const FunctionDecl *HandlerDecl) {
+  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);
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

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

 //===--- CERTTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../bugprone/BadSignalToKillThreadCheck.h"
 #include "../bugprone/ReservedIdentifierCheck.h"
+#include "../bugprone/SignalHandlerCheck.h"
 #include "../bugprone/SignedCharMisuseCheck.h"
 #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h"
 #include "../bugprone/UnhandledSelfAssignmentCheck.h"
 #include "../google/UnnamedNamespaceInHeaderCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<StrToNumCheck>("cert-err34-c");
     // MSC
     CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc30-c");
     CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
         "cert-msc32-c");
     // POS
     CheckFactories.registerCheck<bugprone::BadSignalToKillThreadCheck>(
         "cert-pos44-c");
+    // SIG
+    CheckFactories.registerCheck<bugprone::SignalHandlerCheck>("cert-sig30-c");
     // STR
     CheckFactories.registerCheck<bugprone::SignedCharMisuseCheck>(
         "cert-str34-c");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc
   <clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc>` check.
 
 - New :doc:`bugprone-redundant-branch-condition
   <clang-tidy/checks/bugprone-redundant-branch-condition>` check.
 
   Finds condition variables in nested ``if`` statements that were also checked
   in the outer ``if`` statement and were not changed.
 
+- New :doc:`bugprone-signal-handler
+  <clang-tidy/checks/bugprone-signal-handler>` check.
+
+  Finds functions registered as signal handlers that call non asynchronous-safe
+  functions.
+
+- New :doc:`cert-sig30-c
+  <clang-tidy/checks/cert-sig30-c>` check.
+
+  Alias to the :doc:`bugprone-signal-handler
+  <clang-tidy/checks/bugprone-signal-handler>` check.
+
 - New :doc:`readability-function-cognitive-complexity
   <clang-tidy/checks/readability-function-cognitive-complexity>` check.
 
   Flags functions with Cognitive Complexity metric exceeding the configured limit.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`modernize-loop-convert
   <clang-tidy/checks/modernize-loop-convert>` check.
 
   Now able to transform iterator loops using ``rbegin`` and ``rend`` methods.
 
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability-identifier-naming>` check.
 
   Added an option `GetConfigPerFile` to support including files which use
   different naming styles.
 
   Now renames overridden virtual methods if the method they override has a
   style violation.
   
   Added support for specifying the style of scoped ``enum`` constants. If 
   unspecified, will fall back to the style for regular ``enum`` constants.
 
 - Removed `google-runtime-references` check because the rule it checks does
   not exist in the Google Style Guide anymore.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer

clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst

+.. title:: clang-tidy - bugprone-signal-handler
+
+bugprone-signal-handler
+=======================
+
+Finds functions registered as signal handlers that call non asynchronous-safe
+functions. Any function that cannot be determined to be an asynchronous-safe
+function call is assumed to be non-asynchronous-safe by the checker,
+including user functions for which only the declaration is visible.
+User function calls with visible definition are checked recursively.
+The check handles only C code.
+
+The minimal list of asynchronous-safe system functions is:
+``abort()``, ``_Exit()``, ``quick_exit()`` and ``signal()``
+(for ``signal`` there are additional conditions that are not checked).
+The check accepts only these calls as asynchronous-safe.
+
+This check corresponds to the CERT C Coding Standard rule
+`SIG30-C. Call only asynchronous-safe functions within signal handlers
+<https://www.securecoding.cert.org/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers>`_.

clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst

+.. title:: clang-tidy - cert-sig30-c
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-signal-handler.html
+
+cert-sig30-c
+============
+
+The cert-sig30-c check is an alias, please see
+`bugprone-signal-handler <bugprone-signal-handler.html>`_
+for more information.

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

.. csv-table::
    `abseil-duration-addition <abseil-duration-addition.html>`_, "Yes"
    `abseil-duration-comparison <abseil-duration-comparison.html>`_, "Yes"
    `abseil-duration-conversion-cast <abseil-duration-conversion-cast.html>`_, "Yes"
    `abseil-duration-division <abseil-duration-division.html>`_, "Yes"
    `abseil-duration-factory-float <abseil-duration-factory-float.html>`_, "Yes"
    `abseil-duration-factory-scale <abseil-duration-factory-scale.html>`_, "Yes"
    `abseil-duration-subtraction <abseil-duration-subtraction.html>`_, "Yes"
    `abseil-duration-unnecessary-conversion <abseil-duration-unnecessary-conversion.html>`_, "Yes"
    `abseil-faster-strsplit-delimiter <abseil-faster-strsplit-delimiter.html>`_, "Yes"
    `abseil-no-internal-dependencies <abseil-no-internal-dependencies.html>`_,
    `abseil-no-namespace <abseil-no-namespace.html>`_,
    `abseil-redundant-strcat-calls <abseil-redundant-strcat-calls.html>`_, "Yes"
    `abseil-str-cat-append <abseil-str-cat-append.html>`_, "Yes"
    `abseil-string-find-startswith <abseil-string-find-startswith.html>`_, "Yes"
    `abseil-string-find-str-contains <abseil-string-find-str-contains.html>`_, "Yes"
    `abseil-time-comparison <abseil-time-comparison.html>`_, "Yes"
    `abseil-time-subtraction <abseil-time-subtraction.html>`_, "Yes"
.. csv-table::
    `bugprone-move-forwarding-reference <bugprone-move-forwarding-reference.html>`_, "Yes"
    `bugprone-multiple-statement-macro <bugprone-multiple-statement-macro.html>`_,
    `bugprone-no-escape <bugprone-no-escape.html>`_,
    `bugprone-not-null-terminated-result <bugprone-not-null-terminated-result.html>`_, "Yes"
    `bugprone-parent-virtual-call <bugprone-parent-virtual-call.html>`_, "Yes"
    `bugprone-posix-return <bugprone-posix-return.html>`_, "Yes"
    `bugprone-redundant-branch-condition <bugprone-redundant-branch-condition.html>`_, "Yes"
    `bugprone-reserved-identifier <bugprone-reserved-identifier.html>`_, "Yes"
+   `bugprone-signal-handler <bugprone-signal-handler.html>`_,
    `bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_,
    `bugprone-sizeof-container <bugprone-sizeof-container.html>`_,
    `bugprone-sizeof-expression <bugprone-sizeof-expression.html>`_,
    `bugprone-spuriously-wake-up-functions <bugprone-spuriously-wake-up-functions.html>`_,
    `bugprone-string-constructor <bugprone-string-constructor.html>`_, "Yes"
    `bugprone-string-integer-assignment <bugprone-string-integer-assignment.html>`_, "Yes"
    `bugprone-string-literal-with-embedded-nul <bugprone-string-literal-with-embedded-nul.html>`_,
    `bugprone-suspicious-enum-usage <bugprone-suspicious-enum-usage.html>`_,
.. csv-table::
    `cert-err58-cpp <cert-err58-cpp.html>`_,
    `cert-err60-cpp <cert-err60-cpp.html>`_,
    `cert-flp30-c <cert-flp30-c.html>`_,
    `cert-mem57-cpp <cert-mem57-cpp.html>`_,
    `cert-msc50-cpp <cert-msc50-cpp.html>`_,
    `cert-msc51-cpp <cert-msc51-cpp.html>`_,
    `cert-oop57-cpp <cert-oop57-cpp.html>`_,
    `cert-oop58-cpp <cert-oop58-cpp.html>`_,
+   `cert-sig30-c <cert-sig30-c.html>`_,
    `clang-analyzer-core.DynamicTypePropagation <clang-analyzer-core.DynamicTypePropagation.html>`_,
    `clang-analyzer-core.uninitialized.CapturedBlockVariable <clang-analyzer-core.uninitialized.CapturedBlockVariable.html>`_,
    `clang-analyzer-cplusplus.InnerPointer <clang-analyzer-cplusplus.InnerPointer.html>`_,
    `clang-analyzer-nullability.NullableReturnedFromNonnull <clang-analyzer-nullability.NullableReturnedFromNonnull.html>`_,
    `clang-analyzer-optin.osx.OSObjectCStyleCast <clang-analyzer-optin.osx.OSObjectCStyleCast.html>`_,
    `clang-analyzer-optin.performance.GCDAntipattern <clang-analyzer-optin.performance.GCDAntipattern.html>`_,
    `clang-analyzer-optin.performance.Padding <clang-analyzer-optin.performance.Padding.html>`_,
    `clang-analyzer-optin.portability.UnixAPI <clang-analyzer-optin.portability.UnixAPI.html>`_,

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

+//===--- signal.h - Stub header for tests -----------------------*- 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 _SIGNAL_H_
+#define _SIGNAL_H_
+
+void _sig_ign(int);
+void _sig_dfl(int);
+
+#define SIGINT 1
+#define SIG_IGN _sig_ign
+#define SIG_DFL _sig_dfl
+
+typedef void (*sighandler_t)(int);
+sighandler_t signal(int, sighandler_t);
+
+#endif // _SIGNAL_H_

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

+//===--- stdlib.h - Stub header for tests -----------------------*- 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 _STDLIB_H_
+#define _STDLIB_H_
+
+void abort(void);
+void _Exit(int);
+void quick_exit(int);
+
+void other_call(int);
+
+#endif // _STDLIB_H_

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

+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+#include "signal.h"
+#include "stdio.h"
+#include "stdlib.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 handler__Exit(int) {
+  _Exit(0);
+}
+
+void handler_quick_exit(int) {
+  quick_exit(0);
+}
+
+void handler_other(int) {
+  printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [cert-sig30-c]
+}
+
+void handler_signal(int) {
+  // FIXME: It is only OK to call signal with the current signal number.
+  signal(0, SIG_DFL);
+}
+
+void f_ok() {
+  abort();
+}
+
+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 [cert-sig30-c]
+}
+
+void f_extern();
+
+void handler_ok(int) {
+  f_ok();
+}
+
+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 [cert-sig30-c]
+}
+
+void test() {
+  signal(SIGINT, handler_abort);
+  signal(SIGINT, handler__Exit);
+  signal(SIGINT, handler_quick_exit);
+  signal(SIGINT, handler_signal);
+  signal(SIGINT, handler_other);
+
+  signal(SIGINT, handler_ok);
+  signal(SIGINT, handler_bad);
+  signal(SIGINT, handler_extern);
+
+  signal(SIGINT, quick_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 [cert-sig30-c]
+
+  signal(SIGINT, SIG_IGN);
+  signal(SIGINT, SIG_DFL);
+}