Hi David,
On Mon, May 18, 2020 at 08:45:36AM -0400, David Malcolm wrote:
> Also, m_unsafe_fndecl is a field of signal_unsafe_call, so we can delay
> calling replacement_fn until inside signal_unsafe_call::emit, after the
> warning has been emitted.
>
> It could even become a member function of signal_unsafe_call, giving
> something like this for signal_unsafe_call::emit:
Thanks for all the hints. I adopted all your suggestions and the
warning plus note now looks like:
/opt/local/gcc/bin/gcc -g -O2 -fanalyzer -c bzip2.c
bzip2.c: In function ‘mySIGSEGVorSIGBUScatcher’:
bzip2.c:874:4: warning: call to ‘exit’ from within signal handler [CWE-479]
[-Wanalyzer-unsafe-call-within-signal-handler]
874 | exit(exitValue);
| ^~~~~~~~~~~~~~~
‘main’: events 1-2
|
| 1784 | IntNative main ( IntNative argc, Char *argv[] )
| | ^~~~
| | |
| | (1) entry to ‘main’
|......
| 1800 | smallMode = False;
| | ~~~~~~~~~
| | |
| | (2) registering ‘mySIGSEGVorSIGBUScatcher’ as signal handler
|
event 3
|
|cc1:
| (3): later on, when the signal is delivered to the process
|
+--> ‘mySIGSEGVorSIGBUScatcher’: events 4-5
|
| 816 | void mySIGSEGVorSIGBUScatcher ( IntNative n )
| | ^~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (4) entry to ‘mySIGSEGVorSIGBUScatcher’
|......
| 874 | exit(exitValue);
| | ~~~~~~~~~~~~~~~
| | |
| | (5) call to ‘exit’ from within signal handler
|
bzip2.c:874:4: note: ‘_exit’ is a possible signal-safe alternative for ‘exit’
874 | exit(exitValue);
| ^~~~~~~~~~~~~~~
| _exit
I also added a testcase. How about the attached?
Thanks,
Mark>From de08b1f0e1db85241bce22cdd920b351489af446 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <m...@klomp.org>
Date: Sun, 17 May 2020 23:50:41 +0200
Subject: [PATCH] analyzer: Add exit, and _exit replacement, to sm-signal.
Warn about usage of exit in signal handler and offer _exit as alternative.
gcc/analyzer/ChangeLog:
* sm-signal.cc(signal_unsafe_call::emit): Possibly add
gcc_rich_location note for replacement.
(signal_unsafe_call::get_replacement_fn): New private function.
(get_async_signal_unsafe_fns): Add "exit".
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/signal-exit.c: New testcase.
---
gcc/analyzer/sm-signal.cc | 40 ++++++++++++++++++---
gcc/testsuite/gcc.dg/analyzer/signal-exit.c | 23 ++++++++++++
2 files changed, 59 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/signal-exit.c
diff --git a/gcc/analyzer/sm-signal.cc b/gcc/analyzer/sm-signal.cc
index 5935e229e77c..d267548324e8 100644
--- a/gcc/analyzer/sm-signal.cc
+++ b/gcc/analyzer/sm-signal.cc
@@ -58,6 +58,7 @@ along with GCC; see the file COPYING3. If not see
#include "analyzer/exploded-graph.h"
#include "analyzer/function-set.h"
#include "analyzer/analyzer-selftests.h"
+#include "gcc-rich-location.h"
#if ENABLE_ANALYZER
@@ -123,13 +124,28 @@ public:
bool emit (rich_location *rich_loc) FINAL OVERRIDE
{
+ auto_diagnostic_group d;
diagnostic_metadata m;
/* CWE-479: Signal Handler Use of a Non-reentrant Function. */
m.add_cwe (479);
- return warning_meta (rich_loc, m,
- OPT_Wanalyzer_unsafe_call_within_signal_handler,
- "call to %qD from within signal handler",
- m_unsafe_fndecl);
+ if (warning_meta (rich_loc, m,
+ OPT_Wanalyzer_unsafe_call_within_signal_handler,
+ "call to %qD from within signal handler",
+ m_unsafe_fndecl))
+ {
+ /* If we know a possible alternative function, add a note
+ suggesting the replacement. */
+ if (const char *replacement = get_replacement_fn ())
+ {
+ gcc_rich_location note_rich_loc (gimple_location (m_unsafe_call));
+ note_rich_loc.add_fixit_replace (replacement);
+ inform (¬e_rich_loc,
+ "%qs is a possible signal-safe alternative for %qD",
+ replacement, m_unsafe_fndecl);
+ }
+ return true;
+ }
+ return false;
}
label_text describe_state_change (const evdesc::state_change &change)
@@ -156,6 +172,21 @@ private:
const signal_state_machine &m_sm;
const gcall *m_unsafe_call;
tree m_unsafe_fndecl;
+
+ /* Returns a replacement function as text if it exists. Currently
+ only "exit" has a signal-safe replacement "_exit", which does
+ slightly less, but can be used in a signal handler. */
+ const char *
+ get_replacement_fn ()
+ {
+ gcc_assert (m_unsafe_fndecl && DECL_P (m_unsafe_fndecl));
+
+ if (strcmp ("exit",
+ IDENTIFIER_POINTER (DECL_NAME (m_unsafe_fndecl))) == 0)
+ return "_exit";
+
+ return NULL;
+ }
};
/* signal_state_machine's ctor. */
@@ -259,6 +290,7 @@ get_async_signal_unsafe_fns ()
// TODO: populate this list more fully
static const char * const async_signal_unsafe_fns[] = {
/* This array must be kept sorted. */
+ "exit",
"fprintf",
"free",
"malloc",
diff --git a/gcc/testsuite/gcc.dg/analyzer/signal-exit.c b/gcc/testsuite/gcc.dg/analyzer/signal-exit.c
new file mode 100644
index 000000000000..ed1583d4a920
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/signal-exit.c
@@ -0,0 +1,23 @@
+/* Example of a bad call within a signal handler with replacement
+ alternative. 'handler' calls 'exit', and 'exit' is not allowed
+ from a signal handler. But '_exit' is allowed. */
+
+#include <signal.h>
+#include <stdlib.h>
+
+extern void body_of_program(void);
+
+static void handler(int signum)
+{
+ exit(1); /* { dg-warning "call to 'exit' from within signal handler" } */
+ /* { dg-message "note: '_exit' is a possible signal-safe alternative for 'exit'" "" { target *-*-* } 12 } */
+}
+
+int main(int argc, const char *argv)
+{
+ signal(SIGINT, handler); /* { dg-message "registering 'handler' as signal handler" } */
+
+ body_of_program();
+
+ return 0;
+}
--
2.20.1
