[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

27

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

189

Please add newline.

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

43

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

115

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

36

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

32

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.

41

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

45

Maybe you could use IdentifierInfo instead of string comparisons.

82

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

96

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

32–34

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.

35

re-declaration -> redeclaration

42

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

44

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

45–47

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).

74

Similar issue here with finding functions in the wrong namespace.

118–119

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

136

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

96

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

34

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.

85

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).

98

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

166

How about: Unnamed functions are explicitly not allowed.

172

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

34

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.

85

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

34

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

34

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

34

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

34

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).

42

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

28

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

118–119

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?

135

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

42

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

118–119

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

118–119

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

118–119

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

118–119

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

118–119

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

118–119

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

118–119

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

118–119

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

34

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.

132

Remove commented-out code?

163

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.

176

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

118

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

34

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

34

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

34

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/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 "DontModifyStdNamespaceCheck.h"
 #include "FloatLoopCounter.h"
 #include "LimitedRandomnessCheck.h"
 #include "MutatingCopyCheck.h"
 #include "NonTrivialTypesLibcMemoryCallsCheck.h"
 #include "PostfixOperatorCheck.h"
 #include "ProperlySeededRandomGeneratorCheck.h"
 #include "SetLongJmpCheck.h"
+#include "SignalHandlerCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cert {
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<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/clang-tidy/cert/CMakeLists.txt

add_clang_library(clangTidyCERTModule
   DontModifyStdNamespaceCheck.cpp
   FloatLoopCounter.cpp
   LimitedRandomnessCheck.cpp
   MutatingCopyCheck.cpp
   NonTrivialTypesLibcMemoryCallsCheck.cpp
   PostfixOperatorCheck.cpp
   ProperlySeededRandomGeneratorCheck.cpp
   SetLongJmpCheck.cpp
+  SignalHandlerCheck.cpp
   StaticObjectExceptionCheck.cpp
   StrToNumCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyBugproneModule

clang-tools-extra/clang-tidy/cert/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_CERT_SIGNALHANDLERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNALHANDLERCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "llvm/ADT/StringSet.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Checker for SEI CERT rule SIG30-C
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-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 cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNALHANDLERCHECK_H

clang-tools-extra/clang-tidy/cert/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 cert {
+
+static bool isSystemCall(const FunctionDecl *FD) {
+  // Find a possible redeclaration in system header.
+
+  // It may be possible to use the canonical declaration instead of the loop:
+
+  // 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 llvm::any_of(FD->redecls(), [](const FunctionDecl *D) {
+    return D->getASTContext().getSourceManager().isInSystemHeader(
+        D->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 cert
+} // namespace tidy
+} // namespace clang

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:`cert-sig30-c
+  <clang-tidy/checks/cert-sig30-c>` check.
+
+  Finds functions registered as signal handlers that call non asynchronous-safe
+  functions. For C code only.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - 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.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...
 
 Improvements to pp-trace
 ------------------------
 
 The improvements are...
 
 Clang-tidy visual studio plugin
 -------------------------------

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

+.. title:: clang-tidy - cert-sig30-c
+
+cert-sig30-c
+============
+
+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 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>`_.
+This check handles only C code. For C++ there are different rules (not checked
+by this check).

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::
    `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 signum, sighandler_t handler);
+
+#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 __status);
+void quick_exit(int __status);
+
+void other_call(int);
+
+#endif // _STDLIB_H_

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

+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+// The function should be classified as system call even if there is a
+// declaration before or after the one in a system header.
+int printf(const char *, ...);
+
+typedef void (*sighandler_t)(int);
+sighandler_t signal(int signum, sighandler_t handler);
+
+#include "signal.h"
+#include "stdio.h"
+#include "stdlib.h"
+
+int printf(const char *, ...);
+
+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);
+}