From a240ef2dbdcb4c82604fd01ab27ca147ecc5a23a Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Tue, 2 Apr 2019 11:34:43 -0400
Subject: [PATCH 174/217] FIXME: improve precision of "free of alloca" messages

---
 gcc/analyzer/engine.cc                         |  9 +++++
 gcc/analyzer/hardcoded-state-machine-malloc.cc | 50 ++++++++++++++++++++++----
 gcc/testsuite/gcc.dg/analyzer/malloc-1.c       |  5 ++-
 gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c |  4 +--
 4 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index 0def32f..cfb5efa 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -576,6 +576,15 @@ public:
   {
     m_pending_diagnostic = pd;
     m_emission_id = emission_id;
+
+    /* Call get_desc now, to ensure that any side-effects take place
+       before pending_diagnostic::emit is called.
+       In particular, state_change_event::get_desc can call
+       pending_diagnostic::describe_critical_state_change;
+       free_of_non_heap can use this to tweak the message.
+       (Would be neater to simply capture this within the state).  */
+    label_text desc = get_desc ();
+    desc.maybe_free ();
   }
 
   enum event_kind m_kind;
diff --git a/gcc/analyzer/hardcoded-state-machine-malloc.cc b/gcc/analyzer/hardcoded-state-machine-malloc.cc
index 092fbce..cf61b0d 100644
--- a/gcc/analyzer/hardcoded-state-machine-malloc.cc
+++ b/gcc/analyzer/hardcoded-state-machine-malloc.cc
@@ -237,17 +237,31 @@ class free_of_non_heap : public malloc_diagnostic
 {
 public:
   free_of_non_heap (const malloc_state_machine &sm, tree arg)
-  : malloc_diagnostic (sm, arg)
-  {}
+  : malloc_diagnostic (sm, arg), m_kind (KIND_UNKNOWN)
+  {
+  }
 
   void emit (rich_location *rich_loc) FINAL OVERRIDE
   {
     auto_diagnostic_group d;
     metadata m;
     m.add_cwe (590); /* CWE-590: Free of Memory not on the Heap.  */
-    warning_at (rich_loc, m,
-		"free of %qE which points to memory not on the heap", m_arg);
-    // FIXME: ideally would distinguish the cases in the error message
+    switch (m_kind)
+      {
+      default:
+	gcc_unreachable ();
+      case KIND_UNKNOWN:
+	warning_at (rich_loc, m,
+		    "free of %qE which points to memory not on the heap",
+		    m_arg);
+	break;
+      case KIND_ALLOCA:
+	warning_at (rich_loc, m,
+		    "free of memory allocated on the stack by %qs (%qE)"
+		    " will corrupt the heap",
+		    "alloca", m_arg);
+	break;
+      }
   }
 
   label_text describe_critical_state_change
@@ -257,7 +271,23 @@ public:
      event_id_t event_id)
     FINAL OVERRIDE
   {
-    return label_text::borrow ("pointer is from here"); // TODO
+    /* Attempt to reconstruct what kind of pointer it is.
+       (It seems neater for this to be a part of the state, though).  */
+    if (TREE_CODE (expr) == SSA_NAME)
+      {
+	gimple *def_stmt = SSA_NAME_DEF_STMT (expr);
+	if (gcall *call = dyn_cast <gcall *> (def_stmt))
+	  {
+	    if (is_named_call_p (call, "alloca", 1)
+		|| is_named_call_p (call, "__builtin_alloca", 1))
+	      {
+		m_kind = KIND_ALLOCA;
+		return label_text::borrow
+		  ("memory is allocated on the stack here");
+	      }
+	  }
+      }
+    return label_text::borrow ("pointer is from here");
   }
 
   /* FIXME.  */
@@ -268,6 +298,14 @@ public:
   {
     return formatted_print ("call to %qs here", "free");
   }
+
+private:
+  enum kind
+  {
+    KIND_UNKNOWN,
+    KIND_ALLOCA
+  };
+  enum kind m_kind;
 };
 
 ////////////////////////////////////////////////////////////////////////////
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c
index 420f612..3de1c39 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c
@@ -253,9 +253,8 @@ int *test_23 (int n)
 
 int test_24 (void)
 {
-  void *ptr = alloca (sizeof (int)); /* { dg-message "pointer is from here" } */
-  free (ptr); /* { dg-warning "free of 'ptr' which points to memory not on the heap \\\[CWE-590\\\]" } */
-  /* TODO: more precise messages here.  */
+  void *ptr = alloca (sizeof (int)); /* { dg-message "memory is allocated on the stack here" } */
+  free (ptr); /* { dg-warning "free of memory allocated on the stack by 'alloca' \\('ptr'\\) will corrupt the heap \\\[CWE-590\\\]" } */
 }
 
 int test_25 (void)
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c
index bdc5bb0..d033969 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c
@@ -37,7 +37,7 @@ void test_3 (size_t sz)
 {
   void *ptr;
   if (sz <= LIMIT)
-    ptr = alloca (sz); /* { dg-message "pointer is from here" } */
+    ptr = alloca (sz); /* { dg-message "memory is allocated on the stack here" } */
   else
     ptr = malloc (sz);
 
@@ -46,7 +46,7 @@ void test_3 (size_t sz)
   /* Bug: the "sz <= LIMIT" above should have been "sz < LIMIT",
      so there's a free-of-alloca when sz == LIMIT.  */
   if (sz >= LIMIT)
-    free (ptr); /* { dg-warning "free of 'ptr' which points to memory not on the heap" } */
+    free (ptr); /* { dg-warning "free of memory allocated on the stack by 'alloca'" } */
 }
 /* { dg-bogus "leak of 'ptr'" } */
 /* This can't happen, as "sz > 1024" && "sz <= 1023" is impossible.  */
-- 
1.8.5.3

