This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by balazske on Sep 10 2020, 5:43 AM.

Details

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

Diff Detail

Event Timeline

balazske created this revision.Sep 10 2020, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 5:43 AM
balazske requested review of this revision.Sep 10 2020, 5:43 AM

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.

Eugene.Zelenko added a project: Restricted Project.
balazske updated this revision to Diff 291158.Sep 11 2020, 1:31 AM
  • Code formatting fixes.
  • Updated description.
  • Improved CalledFunctionsCollector.
balazske marked 4 inline comments as done.Sep 11 2020, 1:36 AM

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.

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 updated this revision to Diff 291238.Sep 11 2020, 8:59 AM

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

Eugene.Zelenko added inline comments.Sep 11 2020, 9:08 AM
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 updated this revision to Diff 291524.Sep 14 2020, 3:02 AM

Changed checker messages, reformatted text, rebase.

balazske marked 2 inline comments as done.Sep 14 2020, 3:06 AM
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.)

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.

baloghadamsoftware retitled this revision from [clang-tidy] Add new check for SEI CERT rule SIG30-C. to [clang-tidy] Add new check for SEI CERT rule SIG30-C.Sep 25 2020, 5:44 AM
baloghadamsoftware added a reviewer: gribozavr2.
aaron.ballman added inline comments.Sep 25 2020, 6:00 AM
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 updated this revision to Diff 295538.Oct 1 2020, 5:38 AM

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

balazske marked 12 inline comments as done.Oct 1 2020, 5:45 AM
balazske added inline comments.
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 added inline comments.Oct 1 2020, 12:24 PM
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 marked an inline comment as done.Oct 2 2020, 2:42 AM
balazske added inline comments.
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.

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 added inline comments.Oct 2 2020, 5:58 AM
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 added inline comments.Oct 2 2020, 6:42 AM
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 added inline comments.Oct 2 2020, 6:59 AM
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 this revision to Diff 295840.Oct 2 2020, 8:47 AM

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

balazske marked 9 inline comments as done.Oct 2 2020, 8:55 AM
balazske added inline comments.
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 added inline comments.Oct 2 2020, 9:07 AM
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 added inline comments.Oct 2 2020, 9:16 AM
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 marked 3 inline comments as done.Oct 6 2020, 12:54 AM
balazske added inline comments.
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 added inline comments.Oct 7 2020, 6:41 AM
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 added inline comments.Oct 7 2020, 7:32 AM
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 added inline comments.Oct 9 2020, 2:13 AM
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 added inline comments.Oct 9 2020, 5:24 AM
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 added inline comments.Oct 9 2020, 8:46 AM
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 added inline comments.Oct 9 2020, 11:08 AM
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 added inline comments.Oct 11 2020, 11:44 PM
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 updated this revision to Diff 297828.Oct 13 2020, 5:14 AM

Removed C++ support.
Other small changes.

aaron.ballman added inline comments.Oct 19 2020, 6:49 AM
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 marked an inline comment as done.Oct 28 2020, 12:46 AM
balazske added inline comments.
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 updated this revision to Diff 301215.Oct 28 2020, 3:18 AM

Handled review comments.

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>

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 updated this revision to Diff 302542.Nov 3 2020, 4:32 AM

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

aaron.ballman accepted this revision.Nov 3 2020, 11:02 AM

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.

This revision is now accepted and ready to land.Nov 3 2020, 11:02 AM
balazske updated this revision to Diff 302778.Nov 4 2020, 1:39 AM

Updated according to comments.