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 <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)
Instead of strcmp, you should be able to use id_equal here, making this a bit
simpler.
Marek
