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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 9 inline comments as done.
balazske added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:33
+
+  for (const FunctionDecl *D : Node.redecls())
+    if (D->getASTContext().getSourceManager().isInSystemHeader(
----------------
aaron.ballman wrote:
> balazske wrote:
> > aaron.ballman wrote:
> > > balazske wrote:
> > > > aaron.ballman wrote:
> > > > > 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.
> > > > 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.
> > > > 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.
> > 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.
> > 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.
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).


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:41
+static bool isAllowedSystemCall(const FunctionDecl *FD) {
+  if (!FD->getIdentifier())
+    return true;
----------------
aaron.ballman wrote:
> A function without an identifier is not a system call, so I would have expected this to return `false` based on the function name.
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.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:13
+(for ``signal`` there are additional conditions that are not checked).
+Every other system call is considered as non asynchronous-safe by the checker.
+
----------------
aaron.ballman wrote:
> balazske wrote:
> > aaron.ballman wrote:
> > > 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.`
> > "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.
> 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.
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".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87449/new/

https://reviews.llvm.org/D87449