Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
On Thu, May 21, 2020 at 09:05:09AM -0400, David Malcolm wrote:
> So the above should read something like:
>
> + exit(1); /* { dg-warning "call to 'exit' from within signal handler"
> "warning" } */
> + /* { dg-message "note: '_exit' is a possible signal-safe alternative for
> 'exit'" "replacement note" { target *-*-* } .-1 } */
>
> OK with that change (but please double-check I got the syntax correct!)
You got the syntax correct and now the test PASS message is different
in gcc.sum. Pushed.
Thanks,
Mark
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
On Thu, 2020-05-21 at 00:48 +0200, Mark Wielaard wrote:
> Hi,
>
> On Mon, May 18, 2020 at 07:30:58PM -0400, Marek Polacek wrote:
> > > + /* 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)
> >
> > Instead of strcmp, you should be able to use id_equal here, making
> > this a bit
> > simpler.
>
> That does make it a little easier to read.
> How about the attached?
>
> Thanks,
>
> Mark
> + 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 } */
A couple of nits here:
(a) if two dg diagnostic directives share the same line, it's best to
give them names so that they can be disambiguated in the gcc.sum
output, in case something starts failing for some reason. So please
replace that "" with e.g. "replacement note" and give the warning a
name (e.g. "warning").
(b) rather than hardcoding the absolute line number as 12 above, it's
more future-proof to use a relative line number .-1
So the above should read something like:
+ exit(1); /* { dg-warning "call to 'exit' from within signal handler"
"warning" } */
+ /* { dg-message "note: '_exit' is a possible signal-safe alternative for
'exit'" "replacement note" { target *-*-* } .-1 } */
OK with that change (but please double-check I got the syntax correct!)
Thanks
Dave
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
Hi,
On Mon, May 18, 2020 at 07:30:58PM -0400, Marek Polacek wrote:
> > + /* 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)
>
> Instead of strcmp, you should be able to use id_equal here, making this a bit
> simpler.
That does make it a little easier to read.
How about the attached?
Thanks,
Mark
>From 2406c5a70b407052bc4099a80ecddd4ed3d62385 Mon Sep 17 00:00:00 2001
From: Mark Wielaard
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 | 42 +++--
gcc/testsuite/gcc.dg/analyzer/signal-exit.c | 23 +++
2 files changed, 61 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..c0020321071e 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 )
@@ -156,6 +175,20 @@ private:
const signal_state_machine _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 (id_equal ("exit", DECL_NAME (m_unsafe_fndecl)))
+ return "_exit";
+
+return NULL;
+ }
};
/* signal_state_machine's ctor. */
@@ -259,6 +292,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 ..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
+#include
+
+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
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
On Tue, May 19, 2020 at 01:19:13AM +0200, Mark Wielaard wrote:
> 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
> 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
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
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
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
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
Hi David, On Mon, May 18, 2020 at 11:09:18AM -0400, David Malcolm wrote: > On Mon, 2020-05-18 at 16:47 +0200, Mark Wielaard wrote: > > 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: > > [...] > > 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? > > 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. Thanks, Mark
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
On Mon, 2020-05-18 at 16:47 +0200, Mark Wielaard wrote: > 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. 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? David
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
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
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 (_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 )
@@ -156,6 +172,21 @@ private:
const signal_state_machine _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
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
Hi David, On Sun, 2020-05-17 at 18:53 -0400, David Malcolm wrote: > BTW, it looks like it's using the wrong location for event (2). It > ought to be showing a call to "signal", not an assignment. Please can > you file a bug about this, and attach the source in question so I can > take a look at some point. As briefly discussed on irc, this is an independent issue, it can be shown with pristine gcc 10.1 and bzip2 1.0.8 source code: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95188 A workaround is to use -fanalyzer-fine-grained. Cheers, Mark
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
On Sun, 2020-05-17 at 18:42 -0400, David Malcolm via Gcc-patches wrote:
> On Sun, 2020-05-17 at 18:39 -0400, David Malcolm via Gcc-patches
> wrote:
> > On Mon, 2020-05-18 at 00:05 +0200, Mark Wielaard wrote:
>
> [...snip...]
>
> > How about something like this (though I even haven't checked if it
> > compiles, and am not 100% sure what the wording should be):
> >
> > bool emit (rich_location *rich_loc) FINAL OVERRIDE
> > {
> > diagnostic_metadata m;
> > /* CWE-479: Signal Handler Use of a Non-reentrant Function. */
> > m.add_cwe (479);
>
> ...and there should be this here:
> auto_diagnostic_group d;
>
> to associate the note with the warning.
>
> > if (warning_meta (rich_loc, m,
> > OPT_Wanalyzer_unsafe_call_within_signal_handler,
> > "call to %qD from within signal handler",
> > m_unsafe_fndecl))
> > {
> > if (m_replacement)
> > {
> > gcc_rich_location note_rich_loc (gimple_location
> > (m_unsafe_call));
> > note_rich_loc.add_fixit_replace (m_replacement);
> > inform (_rich_loc, "%qs is a signal-safe replacement
> > for %qD",
> > m_replacement, unsafe_fndecl);
> > }
> > return true;
> > }
> > return false;
> > }
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:
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);
if (warning_meta (rich_loc, m,
OPT_Wanalyzer_unsafe_call_within_signal_handler,
"call to %qD from within signal handler",
m_unsafe_fndecl))
{
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 (_rich_loc, "%qs is a signal-safe replacement for %qD",
replacement, m_unsafe_fndecl);
}
return true;
}
return false;
}
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
On Mon, 2020-05-18 at 00:05 +0200, Mark Wielaard wrote: > Hi, > > While trying out -fanalyzer on the bzip2 source code I noticed that > it > did warn about some unsafe calls in the signal handler, but din't > warn > about the exit call: > https://sourceware.org/pipermail/bzip2-devel/2020q2/000107.html > > It was easy to add exit to the async_signal_unsafe_fns, but since > there is a signal safe _exit call (which is what you should use in a > signal handler, so no unsafe calls are made, like to the at_exit > handlers) I also wanted to add a fixit hint. > > The fixit hint is emitted, but it is somewhat hard to see. Is there a > better way to do this for analyzer warnings so that it is a bit more > prominent? > > This is how it currently looks: > > /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); > |^~~ > |_exit > ‘main’: events 1-2 > | > | 1784 | IntNative main ( IntNative argc, Char *argv[] ) > | | ^~~~ > | | | > | | (1) entry to ‘main’ > |.. > | 1800 |smallMode = False; > | |~ > | || > | |(2) registering ‘mySIGSEGVorSIGBUScatcher’ as signal > handler > | BTW, it looks like it's using the wrong location for event (2). It ought to be showing a call to "signal", not an assignment. Please can you file a bug about this, and attach the source in question so I can take a look at some point. Thanks Dave > 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 >| > > Thanks, > > Mark >
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
On Sun, 2020-05-17 at 18:39 -0400, David Malcolm via Gcc-patches wrote:
> On Mon, 2020-05-18 at 00:05 +0200, Mark Wielaard wrote:
[...snip...]
> How about something like this (though I even haven't checked if it
> compiles, and am not 100% sure what the wording should be):
>
> bool emit (rich_location *rich_loc) FINAL OVERRIDE
> {
> diagnostic_metadata m;
> /* CWE-479: Signal Handler Use of a Non-reentrant Function. */
> m.add_cwe (479);
...and there should be this here:
auto_diagnostic_group d;
to associate the note with the warning.
> if (warning_meta (rich_loc, m,
> OPT_Wanalyzer_unsafe_call_within_signal_handler,
> "call to %qD from within signal handler",
> m_unsafe_fndecl))
> {
> if (m_replacement)
> {
> gcc_rich_location note_rich_loc (gimple_location
> (m_unsafe_call));
> note_rich_loc.add_fixit_replace (m_replacement);
> inform (_rich_loc, "%qs is a signal-safe replacement
> for %qD",
> m_replacement, unsafe_fndecl);
> }
> return true;
> }
> return false;
> }
Re: [RFC] analyzer: Add exit, and _exit replacement, to sm-signal.
On Mon, 2020-05-18 at 00:05 +0200, Mark Wielaard wrote:
> Hi,
>
> While trying out -fanalyzer on the bzip2 source code I noticed that
> it
> did warn about some unsafe calls in the signal handler,
Great.
> but din't warn
> about the exit call:
> https://sourceware.org/pipermail/bzip2-devel/2020q2/000107.html
> It was easy to add exit to the async_signal_unsafe_fns, but since
> there is a signal safe _exit call (which is what you should use in a
> signal handler, so no unsafe calls are made, like to the at_exit
> handlers) I also wanted to add a fixit hint.
I like this idea.
> The fixit hint is emitted, but it is somewhat hard to see. Is there a
> better way to do this for analyzer warnings so that it is a bit more
> prominent?
>
> This is how it currently looks:
>
> /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);
> |^~~
> |_exit
> ‘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
>|
>
> Thanks,
I think this ought to be implemented as a followup "note" diagnostic
via the "inform" API, so that the followup message is separate from the
initial warning, and thus separate from the path...
[...]
> diagnostic_metadata m;
> /* CWE-479: Signal Handler Use of a Non-reentrant Function. */
> m.add_cwe (479);
> +if (m_replacement != NULL)
> + rich_loc->add_fixit_replace (m_replacement);
> return warning_meta (rich_loc, m,
>OPT_Wanalyzer_unsafe_call_within_signal_handle
> r,
>"call to %qD from within signal handler",
How about something like this (though I even haven't checked if it
compiles, and am not 100% sure what the wording should be):
bool emit (rich_location *rich_loc) FINAL OVERRIDE
{
diagnostic_metadata m;
/* CWE-479: Signal Handler Use of a Non-reentrant Function. */
m.add_cwe (479);
if (warning_meta (rich_loc, m,
OPT_Wanalyzer_unsafe_call_within_signal_handler,
"call to %qD from within signal handler",
m_unsafe_fndecl))
{
if (m_replacement)
{
gcc_rich_location note_rich_loc (gimple_location (m_unsafe_call));
note_rich_loc.add_fixit_replace (m_replacement);
inform (_rich_loc, "%qs is a signal-safe replacement for %qD",
m_replacement, unsafe_fndecl);
}
return true;
}
return false;
}
Dave
