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

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd1b2a523191e: [clang-tidy] Add signal-handler-check for SEI 
CERT rule SIG30-C (authored by balazske).

Changed prior to commit:
  https://reviews.llvm.org/D87449?vs=302778=302841#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
@@ -0,0 +1,78 @@
+// 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);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- 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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- 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
+

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

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

balazske updated this revision to Diff 302778.
balazske added a comment.

Updated according to comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
@@ -0,0 +1,78 @@
+// 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);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- 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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- 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_
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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




Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:12-14
+This check corresponds to the CERT C Coding Standard rule
+`SIG30-C. Call only asynchronous-safe functions within signal handlers
+`_.

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



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:15
+`_.
+The check handles only C code.

Same for the C-only nature of the check.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h:20
+typedef void (*sighandler_t)(int);
+sighandler_t signal(int signum, sighandler_t handler);
+

Might as well drop the parameter names here as well.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h:13-14
+void abort(void);
+void _Exit(int __status);
+void quick_exit(int __status);
+

Might as well drop the parameter names here too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske updated this revision to Diff 302542.
balazske added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
@@ -0,0 +1,78 @@
+// 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);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- 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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- 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 // 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.




Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:33
+
+  for (const FunctionDecl *D : Node.redecls())
+if (D->getASTContext().getSourceManager().isInSystemHeader(

balazske wrote:
> balazske wrote:
> > aaron.ballman wrote:
> > > balazske wrote:
> > > > 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).
> > > I don't think that's necessary (we aren't consistent about whether we 
> > > check the entire redecl 

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

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

balazske added a comment.

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?




Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:33
+
+  for (const FunctionDecl *D : Node.redecls())
+if (D->getASTContext().getSourceManager().isInSystemHeader(

balazske wrote:
> aaron.ballman wrote:
> > balazske wrote:
> > > 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).
> > 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  // Most common case will be inclusion directly from 
> > a header, this works fine
> > 
> 

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

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

balazske updated this revision to Diff 301215.
balazske added a comment.

Handled review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c

Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
@@ -0,0 +1,81 @@
+// 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);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- 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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- 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_
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ 

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

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

balazske marked an inline comment 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:
> > > > > 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).
> 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  // 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  // 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  // But at the same time, who does this?
> 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:131
+}
+/*FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))

Remove commented-out code?



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:162
+
+  if (StrictConformingFunctions.count(II->getName()))
+return true;

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.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:175
+   "calling it from a signal handler may be dangerous")
+  << CalledFunction->getNameAsString();
+  diag(SignalCall->getSourceRange().getBegin(),

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.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:33
+
+  for (const FunctionDecl *D : Node.redecls())
+if (D->getASTContext().getSourceManager().isInSystemHeader(

balazske wrote:
> 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 

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

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

balazske updated this revision to Diff 297828.
balazske added a comment.

Removed C++ support.
Other small changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cert-sig30-c_cpp.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c

Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
@@ -0,0 +1,81 @@
+// 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);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- 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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- 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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cert-sig30-c_cpp.h

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

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

balazske added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

aaron.ballman wrote:
> balazske wrote:
> > aaron.ballman wrote:
> > > balazske wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > balazske wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > 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?
> > > > > > > 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?
> > > > > > 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?
> > > > > 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.
> > > > 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.
> > > 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.
> > 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++.
> 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?)
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

balazske wrote:
> aaron.ballman wrote:
> > balazske wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > balazske wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > 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?
> > > > > > 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?
> > > > > 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?
> > > > 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.
> > > 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.
> > 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.
> 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++.
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?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

aaron.ballman wrote:
> balazske wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > balazske wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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?
> > > > > 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?
> > > > 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?
> > > 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.
> > 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.
> 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.
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++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

balazske wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > balazske wrote:
> > > > aaron.ballman wrote:
> > > > > 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?
> > > > 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?
> > > 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?
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

aaron.ballman wrote:
> aaron.ballman wrote:
> > balazske wrote:
> > > aaron.ballman wrote:
> > > > 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?
> > > 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?
> > 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?
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

aaron.ballman wrote:
> balazske wrote:
> > aaron.ballman wrote:
> > > 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?
> > 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?
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

balazske wrote:
> aaron.ballman wrote:
> > 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?
> 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?
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?



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp:52
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);

balazske wrote:
> aaron.ballman wrote:
> > 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?
> > });
> > ```
> 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.
Let's punt on this until we hear back from CERT on whether this rule should be 
supported in C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

aaron.ballman wrote:
> 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?
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?



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp:52
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);

aaron.ballman wrote:
> 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?
> });
> ```
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:41
+static bool isAllowedSystemCall(const FunctionDecl *FD) {
+  if (!FD->getIdentifier())
+return true;

balazske wrote:
> 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.
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 some_global_vector;
void sig_handler(int sig) {
  int  = 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 some_task;
void sig_handler(int sig) {
  some_task(sig); // Who knows what this will execute when it calls operator()()
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:27
+  // Find a possible redeclaration in system header.
+  for (const FunctionDecl *D : FD->redecls())
+if (FD->getASTContext().getSourceManager().isInSystemHeader(

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

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?



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:134
+// At insertion we have already ensured that only function calls are there.
+const FunctionDecl *F = cast(FunctionCall->getCalleeDecl());
+

`const auto *`



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp:52
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);

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?
});
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 

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

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

balazske updated this revision to Diff 295840.
balazske added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cert-sig30-c_cpp.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
@@ -0,0 +1,64 @@
+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+#include "cert-sig30-c_cpp.h"
+#include "stdio.h"
+
+void handler_abort(int) {
+  std::abort();
+}
+
+void handler__Exit(int) {
+  std::_Exit(0);
+}
+
+void handler_quick_exit(int) {
+  std::quick_exit(0);
+}
+
+void handler_bad1(int) {
+  std::something(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'something' may not be asynchronous-safe; calling it from a signal handler may be dangerous [cert-sig30-c]
+}
+
+void handler_bad2(int) {
+  std::SysStruct S;
+  S << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'operator<<' 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.
+  std::signal(0, SIG_DFL);
+  std::signal(0, SIG_IGN);
+}
+
+void handler_nowarn(int) {
+  std::something(2);
+}
+
+// Function called "signal" that is not to be recognized by the checker.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+
+namespace ns {
+void signal(int, callback_t);
+}
+
+struct S {
+  static void signal(int, callback_t);
+};
+
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);
+  std::signal(SIGINT, handler_quick_exit);
+  std::signal(SIGINT, handler_signal);
+  std::signal(SIGINT, handler_bad1);
+  std::signal(SIGINT, handler_bad2);
+
+  // Do not find problems here.
+  signal(SIGINT, handler_nowarn, 1);
+  ns::signal(SIGINT, handler_nowarn);
+  S::signal(SIGINT, handler_nowarn);
+  system_other::signal(SIGINT, handler_nowarn);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
@@ -0,0 +1,81 @@
+// 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: 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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(

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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(

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.



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

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske added a comment.

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 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske marked an inline comment 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:
> 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.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:84
+  functionDecl(hasName(SignalFun), parameterCountIs(2),
+   anyOf(isInStdNamespace(), isTopLevelSystemFunction()));
+  auto HandlerExpr =

aaron.ballman wrote:
> 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).
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).



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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(

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.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:84
+  functionDecl(hasName(SignalFun), parameterCountIs(2),
+   anyOf(isInStdNamespace(), isTopLevelSystemFunction()));
+  auto HandlerExpr =

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:97
+void SignalHandlerCheck::check(const MatchFinder::MatchResult ) {
+  auto *SignalCall = Result.Nodes.getNodeAs("register_call");
+  auto *HandlerDecl = Result.Nodes.getNodeAs("handler_decl");

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:165
+  const IdentifierInfo *II = FD->getIdentifier();
+  // Nonnamed functions are not explicitly allowed.
+  if (!II)

How about: `Unnamed functions are explicitly not allowed.`



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:171
+return true;
+
+  return false;

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:95
+  std::deque> CalledFunctions{
+  {HandlerDecl, HandlerExpr}};
+

baloghadamsoftware wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske updated this revision to Diff 295538.
balazske added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cert-sig30-c_cpp.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
@@ -0,0 +1,64 @@
+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+#include "cert-sig30-c_cpp.h"
+#include "stdio.h"
+
+void handler_abort(int) {
+  std::abort();
+}
+
+void handler__Exit(int) {
+  std::_Exit(0);
+}
+
+void handler_quick_exit(int) {
+  std::quick_exit(0);
+}
+
+void handler_bad1(int) {
+  std::something(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'something' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void handler_bad2(int) {
+  std::SysStruct S;
+  S << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'operator<<' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void handler_signal(int) {
+  // FIXME: It is only OK to call signal with the current signal number.
+  std::signal(0, SIG_DFL);
+  std::signal(0, SIG_IGN);
+}
+
+void handler_nowarn(int) {
+  std::something(2);
+}
+
+// Function called "signal" that is not to be recognized by the checker.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+
+namespace ns {
+void signal(int, callback_t);
+}
+
+struct S {
+  static void signal(int, callback_t);
+};
+
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);
+  std::signal(SIGINT, handler_quick_exit);
+  std::signal(SIGINT, handler_signal);
+  std::signal(SIGINT, handler_bad1);
+  std::signal(SIGINT, handler_bad2);
+
+  // Do not find problems here.
+  signal(SIGINT, handler_nowarn, 1);
+  ns::signal(SIGINT, handler_nowarn);
+  S::signal(SIGINT, handler_nowarn);
+  system_other::signal(SIGINT, handler_nowarn);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
@@ -0,0 +1,65 @@
+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+#include "signal.h"
+#include "stdio.h"
+#include "stdlib.h"
+
+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' is considered as non asynchronous-safe and should not be called from a signal handler [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' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void f_extern();
+
+void handler_ok(int) {
+  f_ok();
+  f_extern();
+}
+
+void handler_bad(int) {
+  f_bad();
+}
+
+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, quick_exit);
+  signal(SIGINT, other_call);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'other_call' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+
+  signal(SIGINT, SIG_IGN);
+  signal(SIGINT, SIG_DFL);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- stdlib.h - Stub header for tests ---*- C++ 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:31-33
+  // This check does not work with function calls in std namespace.
+  if (!FD->isGlobal() || FD->isInStdNamespace())
+return false;

baloghadamsoftware wrote:
> 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.
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.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:34
+return false;
+  // It is assumed that the function has no other re-declaration that is not
+  // in a system header. Otherwise this may produce wrong result.

re-declaration -> redeclaration



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:41
+static bool isAllowedSystemCall(const FunctionDecl *FD) {
+  if (!FD->getIdentifier())
+return true;

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:43
+return true;
+  const StringRef N = FD->getName();
+  if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun)

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:44-46
+  if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun)
+return true;
+  return false;

baloghadamsoftware wrote:
> Maybe you could use `IdentifierInfo` instead of string comparisons.
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).



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:73
+  const auto IsSignalFunction =
+  callee(functionDecl(hasName(SignalFun), parameterCountIs(2)));
+  const auto HandlerAsSecondArg = hasArgument(

Similar issue here with finding functions in the wrong namespace.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+  diag(FunctionCall->getBeginLoc(),
+   "'%0' is considered as non asynchronous-safe and "
+   "should not be called from a signal handler")
+  << FunctionToCheck->getName();

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:135
+[](const FunctionDecl *FD, const CallExpr *CE) {
+  CalledFunctions.push_back(std::make_pair(FD, CE));
+}};

`emplace_back(FD, CE)`?



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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:31
+static bool isSystemCall(const FunctionDecl *FD) {
+  // This check does not work with function calls in std namespace.
+  if (!FD->isGlobal() || FD->isInStdNamespace())

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.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:35
+  // It is assumed that the function has no other re-declaration that is not
+  // in a system header. Otherwise this may produce wrong result.
+  return FD->getASTContext().getSourceManager().isInSystemHeader(

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:40
+
+static bool isAllowedSystemCall(const FunctionDecl *FD) {
+  if (!FD->getIdentifier())

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:44
+  const StringRef N = FD->getName();
+  if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun)
+return true;

Maybe you could use `IdentifierInfo` instead of string comparisons.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:81
+  callExpr(IsSignalFunction, HandlerAsSecondArg).bind("register_call"),
+  this);
+}

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:95
+  std::deque> CalledFunctions{
+  {HandlerDecl, HandlerExpr}};
+

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.



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:1-17
+.. title:: clang-tidy - cert-sig30-c
+
+cert-sig30-c
+
+
+Finds functions registered as signal handlers that call non asynchronous-safe
+functions. User functions called from the handlers are checked too, as far as

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske updated this revision to Diff 291524.
balazske added a comment.

Changed checker messages, reformatted text, rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+#include "signal.h"
+#include "stdio.h"
+#include "stdlib.h"
+
+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' is considered as non asynchronous-safe and should not be called from a signal handler [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' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+}
+
+void f_extern();
+
+void handler_ok(int) {
+  f_ok();
+  f_extern();
+}
+
+void handler_bad(int) {
+  f_bad();
+}
+
+// Function called "signal" that is not to be recognized by the checker.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+
+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, quick_exit);
+  signal(SIGINT, other_call);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'other_call' is considered as non asynchronous-safe and should not be called from a signal handler [cert-sig30-c]
+
+  signal(SIGINT, SIG_IGN);
+  signal(SIGINT, SIG_DFL);
+
+  // Do not find problems here.
+  signal(SIGINT, handler_bad, 1);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- 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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- 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_
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -115,6 +115,7 @@
`cert-msc51-cpp `_,
`cert-oop57-cpp `_,
`cert-oop58-cpp `_,
+   `cert-sig30-c `_,
`clang-analyzer-core.DynamicTypePropagation `_,
`clang-analyzer-core.uninitialized.CapturedBlockVariable `_,

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:94
+
+- New :doc:`cert-sig30-c
+  ` check.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske updated this revision to Diff 291238.
balazske added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+#include "signal.h"
+#include "stdio.h"
+#include "stdlib.h"
+
+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: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [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: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [cert-sig30-c]
+}
+
+void f_extern();
+
+void handler_ok(int) {
+  f_ok();
+  f_extern();
+}
+
+void handler_bad(int) {
+  f_bad();
+}
+
+// Function called "signal" that is not to be recognized by the checker.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+
+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, quick_exit);
+  signal(SIGINT, other_call);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [cert-sig30-c]
+
+  signal(SIGINT, SIG_IGN);
+  signal(SIGINT, SIG_DFL);
+
+  // Do not find problems here.
+  signal(SIGINT, handler_bad, 1);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- 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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- 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_
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -114,6 +114,7 @@
`cert-msc51-cpp `_,
`cert-oop57-cpp `_,
`cert-oop58-cpp `_,
+   `cert-sig30-c `_,
`clang-analyzer-core.DynamicTypePropagation `_,

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Release Notes were not updated yet.




Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:10
+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).
+Every other system call is considered as non asynchronous-safe by the checker.

Please make all strings not longer then 80 symbols.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske marked 4 inline comments as done.
balazske added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske updated this revision to Diff 291158.
balazske added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+#include "signal.h"
+#include "stdio.h"
+#include "stdlib.h"
+
+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: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [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: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [cert-sig30-c]
+}
+
+void f_extern();
+
+void handler_ok(int) {
+  f_ok();
+  f_extern();
+}
+
+void handler_bad(int) {
+  f_bad();
+}
+
+// Function called "signal" that is not to be recognized by the checker.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+
+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, quick_exit);
+  signal(SIGINT, other_call);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [cert-sig30-c]
+
+  signal(SIGINT, SIG_IGN);
+  signal(SIGINT, SIG_DFL);
+
+  // Do not find problems here.
+  signal(SIGINT, handler_bad, 1);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- stdio.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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- stdio.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_
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -114,6 +114,7 @@
`cert-msc51-cpp `_,
`cert-oop57-cpp `_,
`cert-oop58-cpp `_,
+   `cert-sig30-c `_,
`clang-analyzer-core.DynamicTypePropagation `_,
`clang-analyzer-core.uninitialized.CapturedBlockVariable `_,

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please add entry in Release Notes.




Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:26
+namespace {
+constexpr StringRef SignalFun = "signal";
+constexpr StringRef AbortFun = "abort";

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



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:142
+} // namespace clang
\ No newline at end of file


Please add newline.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h:35
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNALHANDLERCHECK_H
\ No newline at end of file


Please add newline.



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:6
+
+This check finds functions registered as signal handlers that call non 
asynchronous-safe functions.
+User functions called from the handlers are checked too, as far as possible.

Please remove `This check`.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:15
 
-   `abseil-duration-addition `_, "Yes"
-   `abseil-duration-comparison `_, "Yes"
-   `abseil-duration-conversion-cast `_, 
"Yes"
-   `abseil-duration-division `_, "Yes"
-   `abseil-duration-factory-float `_, "Yes"
-   `abseil-duration-factory-scale `_, "Yes"
-   `abseil-duration-subtraction `_, "Yes"
-   `abseil-duration-unnecessary-conversion 
`_, "Yes"
-   `abseil-faster-strsplit-delimiter 
`_, "Yes"
+   `abseil-duration-addition `_,
+   `abseil-duration-comparison `_,

A lot of unrelated changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

balazske created this revision.
Herald added subscribers: cfe-commits, martong, phosek, gamesh411, Szelethus, 
dkrupp, xazax.hun, whisperity, mgorny.
Herald added a project: clang.
balazske requested review of this revision.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87449

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.h
  clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
  clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c

Index: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.c
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+
+#include "signal.h"
+#include "stdio.h"
+#include "stdlib.h"
+
+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: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [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: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [cert-sig30-c]
+}
+
+void f_extern();
+
+void handler_ok(int) {
+  f_ok();
+  f_extern();
+}
+
+void handler_bad(int) {
+  f_bad();
+}
+
+// Function called "signal" that is not to be recognized by the checker.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+
+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, quick_exit);
+  signal(SIGINT, other_call);
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: Signal handler potentially calls non asynchronous-safe function. This may result in undefined behavior. [cert-sig30-c]
+
+  signal(SIGINT, SIG_IGN);
+  signal(SIGINT, SIG_DFL);
+
+  // Do not find problems here.
+  signal(SIGINT, handler_bad, 1);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
@@ -0,0 +1,18 @@
+//===--- stdio.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_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -0,0 +1,22 @@
+//===--- stdio.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_
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++