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 (¬e_rich_loc, "%qs is a signal-safe replacement for %qD", m_replacement, unsafe_fndecl); } return true; } return false; } Dave
