diff mbox series

[29/41] analyzer: new file: sm-signal.cc

Message ID 20200108090302.2425-30-dmalcolm@redhat.com
State New
Headers show
Series v5 of analyzer patch kit | expand

Commit Message

David Malcolm Jan. 8, 2020, 9:02 a.m. UTC
Needs review.

Although the comments say "experimental", there are followups (using
function_set) which make this much more production-ready than the
other sm-*.cc (other than malloc).

Changed in v5:
- update ChangeLog path
- updated copyright years to include 2020

New in v4; part of:
  https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00214.html
with various fixups

gcc/analyzer/ChangeLog:
	* sm-signal.cc: New file.
---
 gcc/analyzer/sm-signal.cc | 306 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 306 insertions(+)
 create mode 100644 gcc/analyzer/sm-signal.cc

Comments

Jeff Law Jan. 10, 2020, 4:01 p.m. UTC | #1
On Wed, 2020-01-08 at 04:02 -0500, David Malcolm wrote:
> Needs review.
> 
> Although the comments say "experimental", there are followups (using
> function_set) which make this much more production-ready than the
> other sm-*.cc (other than malloc).
> 
> Changed in v5:
> - update ChangeLog path
> - updated copyright years to include 2020
> 
> New in v4; part of:
>   https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00214.html
> with various fixups
> 
> gcc/analyzer/ChangeLog:
> 	* sm-signal.cc: New file.
OK.  Extending this one seems pretty trivial -- it's just checking for
specific functions we know aren't safe, right?

jeff
>
David Malcolm Jan. 10, 2020, 4:37 p.m. UTC | #2
On Fri, 2020-01-10 at 09:01 -0700, Jeff Law wrote:
> On Wed, 2020-01-08 at 04:02 -0500, David Malcolm wrote:
> > Needs review.
> > 
> > Although the comments say "experimental", there are followups
> > (using
> > function_set) which make this much more production-ready than the
> > other sm-*.cc (other than malloc).
> > 
> > Changed in v5:
> > - update ChangeLog path
> > - updated copyright years to include 2020
> > 
> > New in v4; part of:
> >   https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00214.html
> > with various fixups
> > 
> > gcc/analyzer/ChangeLog:
> > 	* sm-signal.cc: New file.
> OK. 

Thanks.

> Extending this one seems pretty trivial -- it's just checking for
> specific functions we know aren't safe, right?

Indeed, and I've done some followup work on this in the branch:

  "[PATCH 0/4] analyzer: add class function_set and use in various places"
     https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01422.html

(plus I have experimental code in my working copy that can load such
sets on demand from data files, to avoid baking them into the analyzer,
or relying on attributes in old header files)

Dave
diff mbox series

Patch

diff --git a/gcc/analyzer/sm-signal.cc b/gcc/analyzer/sm-signal.cc
new file mode 100644
index 000000000000..0bd31fcf1179
--- /dev/null
+++ b/gcc/analyzer/sm-signal.cc
@@ -0,0 +1,306 @@ 
+/* An experimental state machine, for tracking bad calls from within
+   signal handlers.
+
+   Copyright (C) 2019-2020 Free Software Foundation, Inc.
+   Contributed by David Malcolm <dmalcolm@redhat.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "function.h"
+#include "basic-block.h"
+#include "gimple.h"
+#include "diagnostic-path.h"
+#include "diagnostic-metadata.h"
+#include "analyzer/analyzer.h"
+#include "analyzer/checker-path.h"
+#include "analyzer/exploded-graph.h"
+#include "analyzer/pending-diagnostic.h"
+#include "analyzer/sm.h"
+
+#if ENABLE_ANALYZER
+
+namespace {
+
+/* An experimental state machine, for tracking calls to async-signal-unsafe
+   functions from within signal handlers.  */
+
+class signal_state_machine : public state_machine
+{
+public:
+  signal_state_machine (logger *logger);
+
+  bool inherited_state_p () const FINAL OVERRIDE { return false; }
+
+  bool on_stmt (sm_context *sm_ctxt,
+		const supernode *node,
+		const gimple *stmt) const FINAL OVERRIDE;
+
+  void on_condition (sm_context *sm_ctxt,
+		     const supernode *node,
+		     const gimple *stmt,
+		     tree lhs,
+		     enum tree_code op,
+		     tree rhs) const FINAL OVERRIDE;
+
+  bool can_purge_p (state_t s) const FINAL OVERRIDE;
+
+  /* These states are "global", rather than per-expression.  */
+
+  /* Start state.  */
+  state_t m_start;
+
+  /* State for when we're in a signal handler.  */
+  state_t m_in_signal_handler;
+
+  /* Stop state.  */
+  state_t m_stop;
+};
+
+/* Concrete subclass for describing call to an async-signal-unsafe function
+   from a signal handler.  */
+
+class signal_unsafe_call
+  : public pending_diagnostic_subclass<signal_unsafe_call>
+{
+public:
+  signal_unsafe_call (const signal_state_machine &sm, const gcall *unsafe_call,
+		      tree unsafe_fndecl)
+  : m_sm (sm), m_unsafe_call (unsafe_call), m_unsafe_fndecl (unsafe_fndecl)
+  {
+    gcc_assert (m_unsafe_fndecl);
+  }
+
+  const char *get_kind () const FINAL OVERRIDE { return "signal_unsafe_call"; }
+
+  bool operator== (const signal_unsafe_call &other) const
+  {
+    return m_unsafe_call == other.m_unsafe_call;
+  }
+
+  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);
+    return warning_at (rich_loc, m,
+		       OPT_Wanalyzer_unsafe_call_within_signal_handler,
+		       "call to %qD from within signal handler",
+		       m_unsafe_fndecl);
+  }
+
+  label_text describe_state_change (const evdesc::state_change &change)
+    FINAL OVERRIDE
+  {
+    if (change.is_global_p ()
+	&& change.m_new_state == m_sm.m_in_signal_handler)
+      {
+	function *handler
+	  = change.m_event.m_dst_state.m_region_model->get_current_function ();
+	return change.formatted_print ("registering %qD as signal handler",
+				       handler->decl);
+      }
+    return label_text ();
+  }
+
+  label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE
+  {
+    return ev.formatted_print ("call to %qD from within signal handler",
+			       m_unsafe_fndecl);
+  }
+
+private:
+  const signal_state_machine &m_sm;
+  const gcall *m_unsafe_call;
+  tree m_unsafe_fndecl;
+};
+
+/* signal_state_machine's ctor.  */
+
+signal_state_machine::signal_state_machine (logger *logger)
+: state_machine ("signal", logger)
+{
+  m_start = add_state ("start");
+  m_in_signal_handler = add_state ("in_signal_handler");
+  m_stop = add_state ("stop");
+}
+
+/* Update MODEL for edges that simulate HANDLER_FUN being called as
+   an signal-handler in response to a signal.  */
+
+static void
+update_model_for_signal_handler (region_model *model,
+				 function *handler_fun)
+{
+  /* Purge all state within MODEL.  */
+  *model = region_model ();
+  model->push_frame (handler_fun, NULL, NULL);
+}
+
+/* Custom exploded_edge info: entry into a signal-handler.  */
+
+class signal_delivery_edge_info_t : public exploded_edge::custom_info_t
+{
+public:
+  void print (pretty_printer *pp) FINAL OVERRIDE
+  {
+    pp_string (pp, "signal delivered");
+  }
+
+  void update_model (region_model *model,
+		     const exploded_edge &eedge) FINAL OVERRIDE
+  {
+    update_model_for_signal_handler (model, eedge.m_dest->get_function ());
+  }
+
+  void add_events_to_path (checker_path *emission_path,
+			   const exploded_edge &eedge ATTRIBUTE_UNUSED)
+    FINAL OVERRIDE
+  {
+    emission_path->add_event
+      (new custom_event (UNKNOWN_LOCATION, NULL_TREE, 0,
+			 "later on,"
+			 " when the signal is delivered to the process"));
+  }
+};
+
+/* Concrete subclass of custom_transition for modeling registration of a
+   signal handler and the signal handler later being called.  */
+
+class register_signal_handler : public custom_transition
+{
+public:
+  register_signal_handler (const signal_state_machine &sm,
+			   tree fndecl)
+  : m_sm (sm), m_fndecl (fndecl) {}
+
+  /* Model a signal-handler FNDECL being called at some later point
+     by injecting an edge to a new function-entry node with an empty
+     callstring, setting the 'in-signal-handler' global state
+     on the node.  */
+  void impl_transition (exploded_graph *eg,
+			exploded_node *src_enode,
+			int sm_idx) FINAL OVERRIDE
+  {
+    function *handler_fun = DECL_STRUCT_FUNCTION (m_fndecl);
+    if (!handler_fun)
+      return;
+    program_point entering_handler
+      = program_point::from_function_entry (eg->get_supergraph (),
+					    handler_fun);
+
+    program_state state_entering_handler (eg->get_ext_state ());
+    update_model_for_signal_handler (state_entering_handler.m_region_model,
+				     handler_fun);
+    state_entering_handler.m_checker_states[sm_idx]->set_global_state
+      (m_sm.m_in_signal_handler);
+
+    exploded_node *dst_enode = eg->get_or_create_node (entering_handler,
+						       state_entering_handler,
+						       NULL);
+    if (dst_enode)
+      eg->add_edge (src_enode, dst_enode, NULL, state_change (),
+		    new signal_delivery_edge_info_t ());
+  }
+
+  const signal_state_machine &m_sm;
+  tree m_fndecl;
+};
+
+/* Return true if CALL is known to be unsafe to call from a signal handler.  */
+
+static bool
+signal_unsafe_p (tree callee_fndecl)
+{
+  // TODO: maintain a list of known unsafe functions
+  if (is_named_call_p (callee_fndecl, "fprintf"))
+    return true;
+
+  return false;
+}
+
+/* Implementation of state_machine::on_stmt vfunc for signal_state_machine.  */
+
+bool
+signal_state_machine::on_stmt (sm_context *sm_ctxt,
+			       const supernode *node,
+			       const gimple *stmt) const
+{
+  const state_t global_state = sm_ctxt->get_global_state ();
+  if (global_state == m_start)
+    {
+      if (const gcall *call = dyn_cast <const gcall *> (stmt))
+	if (tree callee_fndecl = sm_ctxt->get_fndecl_for_call (call))
+	  if (is_named_call_p (callee_fndecl, "signal", call, 2))
+	    {
+	      tree handler = gimple_call_arg (call, 1);
+	      if (TREE_CODE (handler) == ADDR_EXPR
+		  && TREE_CODE (TREE_OPERAND (handler, 0)) == FUNCTION_DECL)
+		{
+		  tree fndecl = TREE_OPERAND (handler, 0);
+		  register_signal_handler rsh (*this, fndecl);
+		  sm_ctxt->on_custom_transition (&rsh);
+		}
+	    }
+    }
+  else if (global_state == m_in_signal_handler)
+    {
+      if (const gcall *call = dyn_cast <const gcall *> (stmt))
+	if (tree callee_fndecl = sm_ctxt->get_fndecl_for_call (call))
+	  if (signal_unsafe_p (callee_fndecl))
+	    sm_ctxt->warn_for_state (node, stmt, NULL_TREE, m_in_signal_handler,
+				     new signal_unsafe_call (*this, call,
+							     callee_fndecl));
+    }
+
+  return false;
+}
+
+/* Implementation of state_machine::on_condition vfunc for
+   signal_state_machine.  */
+
+void
+signal_state_machine::on_condition (sm_context *sm_ctxt ATTRIBUTE_UNUSED,
+				    const supernode *node ATTRIBUTE_UNUSED,
+				    const gimple *stmt ATTRIBUTE_UNUSED,
+				    tree lhs ATTRIBUTE_UNUSED,
+				    enum tree_code op ATTRIBUTE_UNUSED,
+				    tree rhs ATTRIBUTE_UNUSED) const
+{
+  // Empty
+}
+
+bool
+signal_state_machine::can_purge_p (state_t s ATTRIBUTE_UNUSED) const
+{
+  return true;
+}
+
+} // anonymous namespace
+
+/* Internal interface to this file. */
+
+state_machine *
+make_signal_state_machine (logger *logger)
+{
+  return new signal_state_machine (logger);
+}
+
+#endif /* #if ENABLE_ANALYZER */