On Mon, May 18, 2020 at 05:24:58PM +0200, Mark Wielaard wrote:
> On Mon, May 18, 2020 at 11:09:18AM -0400, David Malcolm wrote:
> > Overall, I like it, but it looks like there's a problem with the
> > location of the fix-it hint: it looks like it might be replacing the
> > whole of the underlined section of the call, argument, parentheses and
> > all, with "_exit", rather than just the "exit" token. I wonder if the
> > location of that token is still available in the middle-end by the time
> > the analyzer runs.
> >
> > What does -fdiagnostics-generate-patch emit?
>
> --- bzip2.c
> +++ bzip2.c
> @@ -871,7 +871,7 @@
> terribly wrong. Trying to clean up might fail spectacularly. */
>
> if (opMode == OM_Z) setExit(3); else setExit(2);
> - exit(exitValue);
> + _exit;
> }
>
> Hmmm, back to the drawing board it seems.
It seems it is impossible to untangle the gimple call. We do have the
function decl, but this is associated with the declaration of the
function. Since we know the call starts with a known symbol identifier
(that is all on one line), it seems we should be able to truncate the
source_range for the call location to just that prefix. But it is
incredibly hard to manipulate locations. Neither seems it simple to
get the actual text of the location or a range to sanity check what we
are doing.
So I am afraid we have to drop the actual fixit. But we can still add
the note itself. The diagnostic now looks as follows:
/opt/local/install/gcc/bin/gcc -g -O2 -fanalyzer -fanalyzer-fine-grained -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’
|......
| 1816 | signal (SIGSEGV, mySIGSEGVorSIGBUScatcher);
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (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);
| ^~~~~~~~~~~~~~~
What do you think of the attached patch?
Thanks,
Mark>From e47d9c6898b0db7f56cff03096b3fccaeb801efa 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 using exit in signal handler and suggest _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 | 43 +++++++++++++++++++--
gcc/testsuite/gcc.dg/analyzer/signal-exit.c | 23 +++++++++++
2 files changed, 62 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..379f58004219 100644
--- a/gcc/analyzer/sm-signal.cc
+++ b/gcc/analyzer/sm-signal.cc
@@ -123,13 +123,32 @@ 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 ())
+ {
+ location_t note_loc = gimple_location (m_unsafe_call);
+ /* It would be nice to add a fixit, but the gimple call
+ location covers the whole call expression. It isn't
+ currently possible to cut this down to just the call
+ symbol. So the fixit would replace too much.
+ note_rich_loc.add_fixit_replace (replacement); */
+ inform (note_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 +175,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 +293,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
