From 832ebe8994cfe0fa921e8228874a13f65f9c2bc3 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Fri, 22 Mar 2019 15:55:06 -0400
Subject: [PATCH 137/161] FIXME: start implementing custom state-change
 messages

---
 gcc/analyzer/engine.cc                         |  51 +++++++++--
 gcc/analyzer/hardcoded-state-machine-malloc.cc | 112 +++++++++++++++++++------
 gcc/analyzer/hardcoded-state-machine.h         |  28 ++++++-
 gcc/diagnostic-path.h                          |   1 +
 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-10.c  |   2 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-2.c   |   2 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c   |   7 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-paths-2.c |   2 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-paths-3.c |   2 +-
 gcc/testsuite/gcc.dg/analyzer/malloc-paths-7.c |  21 +++++
 10 files changed, 187 insertions(+), 41 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/malloc-paths-7.c

diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 1771112..12fa87d 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -540,7 +540,8 @@ class checker_event : public diagnostic_event
 public:
   checker_event (enum event_kind kind,
 		 location_t loc, tree fndecl, int depth)
-  : m_kind (kind), m_loc (loc), m_fndecl (fndecl), m_depth (depth)
+    : m_kind (kind), m_loc (loc), m_fndecl (fndecl), m_depth (depth),
+      m_pending_diagnostic (NULL)
   {
   }
 
@@ -566,10 +567,16 @@ public:
     return result;
   }
 
+  void set_pending_diagnostic (pending_diagnostic *pd)
+  {
+    m_pending_diagnostic = pd;
+  }
+
   enum event_kind m_kind;
   location_t m_loc;
   tree m_fndecl;
   int m_depth;
+  pending_diagnostic *m_pending_diagnostic;
 };
 
 class debug_event : public checker_event
@@ -676,11 +683,33 @@ public:
 		m_sm.get_state_name (m_to),
 		m_origin));
     else
-      return label_text::take
-	(print ("state of %qE: %qs -> %qs (origin: NULL)",
-		m_var,
-		m_sm.get_state_name (m_from),
-		m_sm.get_state_name (m_to)));
+      {
+	/* If there is no origin, then presumably this
+	   state change came directly from a pattern match.
+	   Attempt to generate a nicer human-readable description.
+	   For greatest precision-of-wording, give the pending diagnostic
+	   a chance to describe this state change (in terms of the
+	   diagnostic).
+	   Note that we only have a pending_diagnostic set on the event once
+	   the diagnostic is about to being emitted, so the description for
+	   an event can change.  */
+	if (m_pending_diagnostic)
+	  {
+	    label_text custom_desc
+	      = m_pending_diagnostic->describe_critical_state_change
+	      (//diagnostic_context *ctxt,
+	       m_var, m_from, m_to);
+	    if (custom_desc.m_buffer)
+	      return custom_desc;
+	  }
+
+	/* Fallback description.  */
+	return label_text::take
+	  (print ("state of %qE: %qs -> %qs (origin: NULL)",
+		  m_var,
+		  m_sm.get_state_name (m_from),
+		  m_sm.get_state_name (m_to)));
+      }
   }
 
   checker_event *clone () const FINAL OVERRIDE
@@ -903,6 +932,14 @@ public:
 #endif
   }
 
+  void set_pending_diagnostic (pending_diagnostic *pd)
+  {
+    checker_event *e;
+    int i;
+    FOR_EACH_VEC_ELT (m_events, i, e)
+      e->set_pending_diagnostic (pd);
+  }
+
   auto_delete_vec<checker_event> m_events;
   int m_stack_depth;
 };
@@ -3162,6 +3199,8 @@ engine::maybe_emit (const candidate_diagnostic &cd)
   expanded_path->add_event (end_of_path);
   // FIXME: ideally would show the full message here, too?
 
+  expanded_path->set_pending_diagnostic (cd.m_pd);
+
   expanded_path->maybe_log (get_logger (), "path for emission");
 
   gcc_rich_location rich_loc (cd.m_stmt->location);
diff --git a/gcc/analyzer/hardcoded-state-machine-malloc.cc b/gcc/analyzer/hardcoded-state-machine-malloc.cc
index 32bd22c..6365eeb 100644
--- a/gcc/analyzer/hardcoded-state-machine-malloc.cc
+++ b/gcc/analyzer/hardcoded-state-machine-malloc.cc
@@ -40,10 +40,24 @@ static const bool DEBUG = false;
 
 ////////////////////////////////////////////////////////////////////////////
 
-class double_free : public pending_diagnostic
+class malloc_diagnostic : public pending_diagnostic
 {
 public:
-  double_free (tree arg) : m_arg (arg) {}
+  malloc_diagnostic (const malloc_state_machine &sm, tree arg)
+  : m_sm (sm), m_arg (arg)
+  {}
+
+protected:
+  const malloc_state_machine &m_sm;
+  tree m_arg;
+};
+
+class double_free : public malloc_diagnostic
+{
+public:
+  double_free (const malloc_state_machine &sm, tree arg)
+  : malloc_diagnostic (sm, arg)
+  {}
 
   void emit (rich_location *rich_loc) FINAL OVERRIDE
   {
@@ -52,14 +66,36 @@ public:
     warning_at (rich_loc, m, "double-free of %qE", m_arg);
   }
 
-private:
-  tree m_arg;
+  label_text describe_critical_state_change (tree expr,
+					     state_machine::state_t old_state,
+					     state_machine::state_t new_state)
+    FINAL OVERRIDE
+  {
+    if (old_state == m_sm.m_start && new_state == m_sm.m_free)
+      {
+	pretty_printer pp;
+	pp_printf (&pp, "first %qs here", "free");
+	return label_text::take (xstrdup (pp_formatted_text (&pp)));
+	// TODO: using appropriate colorization etc
+      }
+    return label_text ();
+  }
+
+  /* FIXME: maybe also something like this.  */
+#if 0
+  label_test label_critical_state (int event_idx)
+  //FINAL OVERRIDE
+  {
+    return label_text::take (xvsprintf ("freed at (%i)"));
+  }
+#endif
 };
 
-class possible_null_deref : public pending_diagnostic
+class possible_null_deref : public malloc_diagnostic
 {
 public:
-  possible_null_deref (tree arg) : m_arg (arg) {}
+  possible_null_deref (const malloc_state_machine &sm, tree arg)
+  : malloc_diagnostic (sm, arg) {}
 
   void emit (rich_location *rich_loc) FINAL OVERRIDE
   {
@@ -69,14 +105,22 @@ public:
     warning_at (rich_loc, m, "use of possibly-NULL %qE", m_arg);
   }
 
-private:
-  tree m_arg;
+  label_text describe_critical_state_change (tree expr,
+					     state_machine::state_t old_state,
+					     state_machine::state_t new_state)
+    FINAL OVERRIDE
+  {
+    if (old_state == m_sm.m_start && new_state == m_sm.m_unchecked)
+      return label_text::borrow ("this call could return NULL");
+    return label_text ();
+  }
 };
 
-class null_deref : public pending_diagnostic
+class null_deref : public malloc_diagnostic
 {
 public:
-  null_deref (tree arg) : m_arg (arg) {}
+  null_deref (const malloc_state_machine &sm, tree arg)
+  : malloc_diagnostic (sm, arg) {}
 
   void emit (rich_location *rich_loc) FINAL OVERRIDE
   {
@@ -85,15 +129,14 @@ public:
     m.add_cwe (690);
     warning_at (rich_loc, m, "use of NULL %qE", m_arg);
   }
-
-private:
-  tree m_arg;
 };
 
-class use_after_free : public pending_diagnostic
+class use_after_free : public malloc_diagnostic
 {
 public:
-  use_after_free (tree arg) : m_arg (arg) {}
+  use_after_free (const malloc_state_machine &sm, tree arg)
+  : malloc_diagnostic (sm, arg)
+  {}
 
   void emit (rich_location *rich_loc) FINAL OVERRIDE
   {
@@ -103,14 +146,22 @@ public:
     warning_at (rich_loc, m, "use after free of %qE", m_arg);
   }
 
-private:
-  tree m_arg;
+  label_text describe_critical_state_change (tree expr,
+					     state_machine::state_t old_state,
+					     state_machine::state_t new_state)
+    FINAL OVERRIDE
+  {
+    if (new_state == m_sm.m_free)
+      return label_text::borrow ("freed here");
+    return label_text ();
+  }
 };
 
-class malloc_leak : public pending_diagnostic
+class malloc_leak : public malloc_diagnostic
 {
 public:
-  malloc_leak (tree arg) : m_arg (arg) {}
+  malloc_leak (const malloc_state_machine &sm, tree arg)
+  : malloc_diagnostic (sm, arg) {}
 
   void emit (rich_location *rich_loc) FINAL OVERRIDE
   {
@@ -119,8 +170,15 @@ public:
     warning_at (rich_loc, m, "leak of %qE", m_arg);
   }
 
-private:
-  tree m_arg;
+  label_text describe_critical_state_change (tree expr,
+					     state_machine::state_t old_state,
+					     state_machine::state_t new_state)
+    FINAL OVERRIDE
+  {
+    if (new_state == m_sm.m_unchecked)
+      return label_text::borrow ("allocated here");
+    return label_text ();
+  }
 };
 
 ////////////////////////////////////////////////////////////////////////////
@@ -177,7 +235,7 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt,
 
 	  /* free -> stop, with warning.  */
 	  sm_ctxt->warn_for_state (node, stmt, arg, m_free,
-				   new double_free (arg));
+				   new double_free (*this, arg));
 	  sm_ctxt->on_transition (node, stmt, arg, m_free, m_stop);
 	}
     }
@@ -212,15 +270,15 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt,
 	  tree arg = TREE_OPERAND (op, 0);
 
 	  sm_ctxt->warn_for_state (node, stmt, arg, m_unchecked,
-				   new possible_null_deref (arg));
+				   new possible_null_deref (*this, arg));
 	  sm_ctxt->on_transition (node, stmt, arg, m_unchecked, m_nonnull);
 
 	  sm_ctxt->warn_for_state (node, stmt, arg, m_null,
-				   new null_deref (arg));
+				   new null_deref (*this, arg));
 	  sm_ctxt->on_transition (node, stmt, arg, m_null, m_stop);
 
 	  sm_ctxt->warn_for_state (node, stmt, arg, m_free,
-				   new use_after_free (arg));
+				   new use_after_free (*this, arg));
 	  sm_ctxt->on_transition (node, stmt, arg, m_free, m_stop);
 	}
     }
@@ -277,7 +335,7 @@ malloc_state_machine::on_leak (sm_context *sm_ctxt,
 			       state_machine::state_t state) const
 {
   sm_ctxt->warn_for_state (node, stmt, var, m_unchecked,
-			   new malloc_leak (var));
+			   new malloc_leak (*this, var));
   sm_ctxt->warn_for_state (node, stmt, var, m_nonnull,
-			   new malloc_leak (var));
+			   new malloc_leak (*this, var));
 }
diff --git a/gcc/analyzer/hardcoded-state-machine.h b/gcc/analyzer/hardcoded-state-machine.h
index 72a97c9..7078011 100644
--- a/gcc/analyzer/hardcoded-state-machine.h
+++ b/gcc/analyzer/hardcoded-state-machine.h
@@ -104,6 +104,33 @@ class pending_diagnostic
  public:
   virtual ~pending_diagnostic () {}
   virtual void emit (rich_location *) = 0;
+
+  /* For greatest precision-of-wording, give the pending diagnostic
+     a chance to describe critical state changes in terms of the
+     diagnostic.
+
+     For example, a double-free diagnostic might use the descriptions:
+     - "first 'free' happens here"
+     - "second 'free' happens here"
+     for the pertinent events, whereas a use-after-free might use the
+     descriptions:
+     - "freed here"
+     - "use after free here"
+     Note how in both cases the first event is a "free": the best
+     description to use depends on the diagnostic.
+
+     Return a non-NULL label_text to give the state change a custom
+     description; NULL otherwise.  */
+
+  virtual label_text describe_critical_state_change
+    (/*diagnostic_context *ctxt, */
+     tree expr ATTRIBUTE_UNUSED,
+     state_machine::state_t old_state ATTRIBUTE_UNUSED,
+     state_machine::state_t new_state ATTRIBUTE_UNUSED)
+  {
+    /* Default no-op implementation.  */
+    return label_text ();
+  }
 };
 
 #if 0
@@ -176,7 +203,6 @@ public:
 		tree var,
 		state_machine::state_t state) const FINAL OVERRIDE;
 
-private:
   state_t m_start;
   state_t m_unchecked;
   state_t m_null;
diff --git a/gcc/diagnostic-path.h b/gcc/diagnostic-path.h
index 4eea07e..b337feb 100644
--- a/gcc/diagnostic-path.h
+++ b/gcc/diagnostic-path.h
@@ -73,6 +73,7 @@ class diagnostic_event
      calls, returns, and frame nesting.  */
   virtual int get_stack_depth () const = 0;
 
+  /* Get a localized (and possibly colorized) description of this event.  */
   virtual label_text get_desc () const = 0;
 };
 
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-10.c b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-10.c
index d10c50d..e59faa2 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-10.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-10.c
@@ -5,7 +5,7 @@ calls_free (void *victim) /* { dg-message "\\(3\\) entry to 'calls_free'" } */
 /* { dg-message "\\(7\\) entry to 'calls_free'" "" { target *-*-* } .-1 } */
 {
   free (victim); /* { dg-warning "double-free of 'victim'" } */
-  /* { dg-message "\\(4\\) state of 'victim': 'start' -> 'free'" "" { target *-*-* } .-1 } */
+  /* { dg-message "\\(4\\) first 'free' here" "" { target *-*-* } .-1 } */
   /* { dg-message "\\(8\\) here \\('victim' is in state 'free'\\)" "" { target *-*-* } .-2 } */
 
   /* TODO: would this be better emitted at the callsite,
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-2.c b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-2.c
index 55b77d2..508cfdc 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-2.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-2.c
@@ -7,7 +7,7 @@ calls_free (void *victim) /* { dg-message "\\(3\\) entry to 'calls_free'" } */
 /* { dg-message "\\(7\\) entry to 'calls_free'" "" { target *-*-* } .-1 } */
 {
   free (victim); /* { dg-warning "double-free of 'victim'" } */
-  /* { dg-message "\\(4\\) state of 'victim': 'start' -> 'free'" "" { target *-*-* } .-1 } */
+  /* { dg-message "\\(4\\) first 'free' here" "" { target *-*-* } .-1 } */
   /* { dg-message "\\(8\\) here \\('victim' is in state 'free'\\)" "" { target *-*-* } .-2 } */
 
   /* TODO: would this be better emitted at the callsite,
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c
index 9f99287..d222e04 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8.c
@@ -17,7 +17,7 @@ void wrapped_free (void *ptr) /* { dg-warning "summary: add: 'ptr': 'start' -> '
 /* { dg-message "\\(17\\) entry to 'wrapped_free'" "" { target *-*-* } .-4 } */
 {
   free (ptr); /* { dg-warning "double-free of 'ptr' \\\[CWE-415\\]" } */
-  /* { dg-message "\\(11\\) state of 'ptr': 'start' -> 'free'" "" { target *-*-* } .-1 } */
+  /* { dg-message "\\(11\\) first 'free' here" "" { target *-*-* } .-1 } */
   /* { dg-message "\\(12\\) state of 'ptr': 'unchecked' -> 'free'" "" { target *-*-* } .-2 } */
   /* FIXME: unchecked -> free is correct, but start -> free should be pruned
      from the path.  */
@@ -45,10 +45,11 @@ free_boxed_int (boxed_int *bi) /* { dg-warning "summary: add: 'bi': 'start' -> '
 /* { dg-message "\\(15\\) entry to 'free_boxed_int'" "" { target *-*-* } .-3 } */
 {
   wrapped_free (bi); /* { dg-message "\\(8\\) state of 'ptr': 'start' -> 'unchecked'" } */
-  /* { dg-message "\\(9\\) state of 'ptr': 'start' -> 'free'" "" { target *-*-* } .-1 } */
+  /* { dg-message "\\(9\\) first 'free' here" "" { target *-*-* } .-1 } */
   /* FIXME: these are wrong; they refer to ptr, not bi.
+     (was "state of 'ptr': 'start' -> 'free'")
      Also, they should be just start->free, or be skipped.  */
-  /* { dg-message "\\(16\\) calling 'wrapped_free' from 'free_boxed_int'" "" { target *-*-* } .-4 } */
+  /* { dg-message "\\(16\\) calling 'wrapped_free' from 'free_boxed_int'" "" { target *-*-* } .-5 } */
 }
 
 void test (int i) /* { dg-message "\\(1\\) entry to 'test'" } */
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-2.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-2.c
index e0e40fe..c0cd5a8 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-2.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-2.c
@@ -8,6 +8,6 @@ void test_2 (void *ptr)
   free (ptr); /* { dg-line second_free } */
 
   /* { dg-warning "double-free of 'ptr'"  "" { target *-*-* } second_free } */
-  /* { dg-message "\\(1\\) state of 'ptr': 'start' -> 'free'" "" { target *-*-* } first_free } */
+  /* { dg-message "\\(1\\) first 'free' here" "" { target *-*-* } first_free } */
   /* { dg-message "\\(2\\) here \\('ptr' is in state 'free'\\)" "" { target *-*-* } second_free } */
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-3.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-3.c
index 2de2ba3..eb88968 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-3.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-3.c
@@ -9,6 +9,6 @@ int *test_3 (void)
   return ptr;
 
   /* { dg-warning "use of possibly-NULL 'ptr'" "" { target *-*-* } unchecked_deref } */
-  /* { dg-message "\\(1\\) state of 'ptr': 'start' -> 'unchecked'" "" { target *-*-* } malloc } */
+  /* { dg-message "\\(1\\) this call could return NULL" "" { target *-*-* } malloc } */
   /* { dg-message "\\(2\\) here \\('ptr' is in state 'unchecked'\\)" "" { target *-*-* } unchecked_deref } */
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-7.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-7.c
new file mode 100644
index 0000000..7f5e322
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-7.c
@@ -0,0 +1,21 @@
+#include <stdlib.h>
+
+extern int foo (void);
+extern int bar (void);
+
+void test (void)
+{
+  void *p = malloc (1024); /* { dg-message "\\(1\\) allocated here" } */ 
+  void *q = malloc (1024);
+
+  foo ();
+  if (!q)
+    {
+      free (q);
+      return; /* { dg-warning "leak of 'p'" } */ 
+      /* { dg-message "\\(2\\) here" "" { target *-*-* } .-1 } */
+    }
+  bar ();
+  free (q);
+  free (p);
+}
-- 
1.8.5.3

