[07/11,analyzer] Fix missing leak on longjmp past a free

Message ID 1574284430-8776-8-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • Static analysis v2
Related show

Commit Message

David Malcolm Nov. 20, 2019, 9:13 p.m.
This patch fixes the missing leak report in setjmp-7.c, which failed
to report on an unchecked "malloc" result leaking due to a longjmp
skipping past its frame to a setjmp in its caller.

The root cause issue is that impl_region_model_context::on_state_leak
would call the state machine's on_leak vfunc with a tree; this would
call back into impl_region_model_context's warn_for_state vfunc, which
would add a leak pending_diagnostic if the tree was in the correct
sm-state.

The latter check was redundant: we already know that the expression in
an sm-state for which we ought to report a leak.  The check was getting
the wrong result: by passing around a tree (for the SSA_NAME for the
result of "malloc" in the "middle" frame), the state-lookup was implicitly
converting this back to a path_var, and looking for the SSA_NAME's state
in the "inner" frame.  I looked at updating warn_for_state to take a
path_var rather than a tree, but this led to further cleanups being needed
in on_transition.

This patch takes the simpler approach of having on_leak only be called
once a leak of a report-worthy state has been detected, and having it
return a pending_diagnostic instance, eliminating the redundant state
check.

This simplification fixes the missing leak report (although the readability
of the report could be improved; it's missing a description of where the
longjmp is rewinding to.  I'll leave that for a followup).

gcc/ChangeLog:
	* analyzer/engine.cc (impl_region_model_context::on_state_leak):
	Rather than calling sm.on_leak and having it call back with
	warn_for_state, get pending_diagnostic from return value of
	on_leak, and add it directly, avoiding a redundant check of
	the state of the var.
	* analyzer/sm-file.cc (fileptr_state_machine::on_leak): Update
	for simpler API.
	* analyzer/sm-malloc.cc (malloc_state_machine::on_leak): Likewise.
	* analyzer/sm-pattern-test.cc
	(pattern_test_state_machine::on_leak): Delete.
	* analyzer/sm-sensitive.cc (sensitive_state_machine::on_leak):
	Delete.
	* analyzer/sm-taint.cc (taint_state_machine::on_leak): Delete.
	* analyzer/sm.h (state_machine::on_leak): Convert return type
	from "void" to "pending_diagnostic *".  Drop all parameters except
	tree.  Provide empty base class	implementation.

gcc/testsuite/ChangeLog:
	* gcc.dg/analyzer/setjmp-7.c: Remove xfails.
	* gcc.dg/analyzer/setjmp-7a.c: New test.
---
 gcc/analyzer/engine.cc                    |   8 ++-
 gcc/analyzer/sm-file.cc                   |  34 ++++------
 gcc/analyzer/sm-malloc.cc                 |  33 ++++------
 gcc/analyzer/sm-pattern-test.cc           |  17 -----
 gcc/analyzer/sm-sensitive.cc              |  16 -----
 gcc/analyzer/sm-taint.cc                  |  16 -----
 gcc/analyzer/sm.h                         |  12 ++--
 gcc/testsuite/gcc.dg/analyzer/setjmp-7.c  |   4 +-
 gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c | 101 ++++++++++++++++++++++++++++++
 9 files changed, 137 insertions(+), 104 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c

-- 
1.8.5.3

Patch

diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc
index c4ca5bf..9936e1e 100644
--- a/gcc/analyzer/engine.cc
+++ b/gcc/analyzer/engine.cc
@@ -521,8 +521,12 @@  impl_region_model_context::on_state_leak (const state_machine &sm,
 	}
     }
 
-  sm.on_leak (&sm_ctxt, m_enode_for_diag->get_supernode (), m_stmt,
-	      leaked_tree, state);
+  pending_diagnostic *pd = sm.on_leak (leaked_tree);
+  if (pd)
+    m_eg->get_diagnostic_manager ().add_diagnostic
+      (&sm, m_enode_for_diag, m_enode_for_diag->get_supernode (),
+       m_stmt, &stmt_finder,
+       leaked_tree, state, pd);
 }
 
 /* Implementation of region_model_context::on_inherited_svalue vfunc
diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc
index 4fc308c..f2e8030 100644
--- a/gcc/analyzer/sm-file.cc
+++ b/gcc/analyzer/sm-file.cc
@@ -52,12 +52,8 @@  public:
 		     enum tree_code op,
 		     tree rhs) const FINAL OVERRIDE;
 
-  void on_leak (sm_context *sm_ctxt,
-		const supernode *node,
-		const gimple *stmt,
-		tree var,
-		state_machine::state_t state) const FINAL OVERRIDE;
   bool can_purge_p (state_t s) const FINAL OVERRIDE;
+  pending_diagnostic *on_leak (tree var) const FINAL OVERRIDE;
 
   /* Start state.  */
   state_t m_start;
@@ -299,24 +295,6 @@  fileptr_state_machine::on_condition (sm_context *sm_ctxt,
     }
 }
 
-/* Implementation of state_machine::on_leak vfunc for
-   fileptr_state_machine.
-   Complain about leaks of FILE * in state 'unchecked' and 'nonnull'.  */
-
-void
-fileptr_state_machine::on_leak (sm_context *sm_ctxt,
-				const supernode *node,
-				const gimple *stmt,
-				tree var,
-				state_machine::state_t state ATTRIBUTE_UNUSED)
-  const
-{
-  sm_ctxt->warn_for_state (node, stmt, var, m_unchecked,
-			   new file_leak (*this, var));
-  sm_ctxt->warn_for_state (node, stmt, var, m_nonnull,
-			   new file_leak (*this, var));
-}
-
 /* Implementation of state_machine::can_purge_p vfunc for fileptr_state_machine.
    Don't allow purging of pointers in state 'unchecked' or 'nonnull'
    (to avoid false leak reports).  */
@@ -327,6 +305,16 @@  fileptr_state_machine::can_purge_p (state_t s) const
   return s != m_unchecked && s != m_nonnull;
 }
 
+/* Implementation of state_machine::on_leak vfunc for
+   fileptr_state_machine, for complaining about leaks of FILE * in
+   state 'unchecked' and 'nonnull'.  */
+
+pending_diagnostic *
+fileptr_state_machine::on_leak (tree var) const
+{
+  return new file_leak (*this, var);
+}
+
 } // anonymous namespace
 
 /* Internal interface to this file. */
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index 4e02f37..602cb0b 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -54,12 +54,8 @@  public:
 		     enum tree_code op,
 		     tree rhs) const FINAL OVERRIDE;
 
-  void on_leak (sm_context *sm_ctxt,
-		const supernode *node,
-		const gimple *stmt,
-		tree var,
-		state_machine::state_t state) const FINAL OVERRIDE;
   bool can_purge_p (state_t s) const FINAL OVERRIDE;
+  pending_diagnostic *on_leak (tree var) const FINAL OVERRIDE;
 
   /* Start state.  */
   state_t m_start;
@@ -761,23 +757,6 @@  malloc_state_machine::on_condition (sm_context *sm_ctxt,
     }
 }
 
-/* Implementation of state_machine::on_leak vfunc for malloc_state_machine.
-   Complain about leaks of pointers in state 'unchecked' and 'nonnull'.  */
-
-void
-malloc_state_machine::on_leak (sm_context *sm_ctxt,
-			       const supernode *node,
-			       const gimple *stmt,
-			       tree var,
-			       state_machine::state_t state ATTRIBUTE_UNUSED)
-  const
-{
-  sm_ctxt->warn_for_state (node, stmt, var, m_unchecked,
-			   new malloc_leak (*this, var));
-  sm_ctxt->warn_for_state (node, stmt, var, m_nonnull,
-			   new malloc_leak (*this, var));
-}
-
 /* Implementation of state_machine::can_purge_p vfunc for malloc_state_machine.
    Don't allow purging of pointers in state 'unchecked' or 'nonnull'
    (to avoid false leak reports).  */
@@ -788,6 +767,16 @@  malloc_state_machine::can_purge_p (state_t s) const
   return s != m_unchecked && s != m_nonnull;
 }
 
+/* Implementation of state_machine::on_leak vfunc for malloc_state_machine
+   (for complaining about leaks of pointers in state 'unchecked' and
+   'nonnull').  */
+
+pending_diagnostic *
+malloc_state_machine::on_leak (tree var) const
+{
+  return new malloc_leak (*this, var);
+}
+
 } // anonymous namespace
 
 /* Internal interface to this file. */
diff --git a/gcc/analyzer/sm-pattern-test.cc b/gcc/analyzer/sm-pattern-test.cc
index cf42cfd..746f2fa 100644
--- a/gcc/analyzer/sm-pattern-test.cc
+++ b/gcc/analyzer/sm-pattern-test.cc
@@ -56,11 +56,6 @@  public:
 		     enum tree_code op,
 		     tree rhs) const FINAL OVERRIDE;
 
-  void on_leak (sm_context *sm_ctxt,
-		const supernode *node,
-		const gimple *stmt,
-		tree var,
-		state_machine::state_t state) const FINAL OVERRIDE;
   bool can_purge_p (state_t s) const FINAL OVERRIDE;
 
 private:
@@ -136,18 +131,6 @@  pattern_test_state_machine::on_condition (sm_context *sm_ctxt,
   sm_ctxt->warn_for_state (node, stmt, lhs, m_start, diag);
 }
 
-void
-pattern_test_state_machine::on_leak (sm_context *sm_ctxt ATTRIBUTE_UNUSED,
-				     const supernode *node ATTRIBUTE_UNUSED,
-				     const gimple *stmt ATTRIBUTE_UNUSED,
-				     tree var ATTRIBUTE_UNUSED,
-				     state_machine::state_t state
-				       ATTRIBUTE_UNUSED)
-  const
-{
-  /* Empty.  */
-}
-
 bool
 pattern_test_state_machine::can_purge_p (state_t s ATTRIBUTE_UNUSED) const
 {
diff --git a/gcc/analyzer/sm-sensitive.cc b/gcc/analyzer/sm-sensitive.cc
index f634b8f..414dd56 100644
--- a/gcc/analyzer/sm-sensitive.cc
+++ b/gcc/analyzer/sm-sensitive.cc
@@ -54,11 +54,6 @@  public:
 		     enum tree_code op,
 		     tree rhs) const FINAL OVERRIDE;
 
-  void on_leak (sm_context *sm_ctxt,
-		const supernode *node,
-		const gimple *stmt,
-		tree var,
-		state_machine::state_t state) const FINAL OVERRIDE;
   bool can_purge_p (state_t s) const FINAL OVERRIDE;
 
 private:
@@ -181,17 +176,6 @@  sensitive_state_machine::on_condition (sm_context *sm_ctxt ATTRIBUTE_UNUSED,
   /* Empty.  */
 }
 
-void
-sensitive_state_machine::on_leak (sm_context *sm_ctxt ATTRIBUTE_UNUSED,
-				  const supernode *node ATTRIBUTE_UNUSED,
-				  const gimple *stmt ATTRIBUTE_UNUSED,
-				  tree var ATTRIBUTE_UNUSED,
-				  state_machine::state_t state ATTRIBUTE_UNUSED)
-  const
-{
-  /* Empty.  */
-}
-
 bool
 sensitive_state_machine::can_purge_p (state_t s ATTRIBUTE_UNUSED) const
 {
diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index c664a54..59b9171 100644
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -55,11 +55,6 @@  public:
 		     enum tree_code op,
 		     tree rhs) const FINAL OVERRIDE;
 
-  void on_leak (sm_context *sm_ctxt,
-		const supernode *node,
-		const gimple *stmt,
-		tree var,
-		state_machine::state_t state) const FINAL OVERRIDE;
   bool can_purge_p (state_t s) const FINAL OVERRIDE;
 
   /* Start state.  */
@@ -310,17 +305,6 @@  taint_state_machine::on_condition (sm_context *sm_ctxt,
     }
 }
 
-void
-taint_state_machine::on_leak (sm_context *sm_ctxt ATTRIBUTE_UNUSED,
-			      const supernode *node ATTRIBUTE_UNUSED,
-			      const gimple *stmt ATTRIBUTE_UNUSED,
-			      tree var ATTRIBUTE_UNUSED,
-			      state_machine::state_t state ATTRIBUTE_UNUSED)
-  const
-{
-  /* Empty.  */
-}
-
 bool
 taint_state_machine::can_purge_p (state_t s ATTRIBUTE_UNUSED) const
 {
diff --git a/gcc/analyzer/sm.h b/gcc/analyzer/sm.h
index 0a89f8a..b35d8c5 100644
--- a/gcc/analyzer/sm.h
+++ b/gcc/analyzer/sm.h
@@ -73,17 +73,17 @@  public:
 			     const gimple *stmt,
 			     tree lhs, enum tree_code op, tree rhs) const = 0;
 
-  virtual void on_leak (sm_context *sm_ctxt,
-			const supernode *node,
-			const gimple *stmt,
-			tree var,
-			state_machine::state_t state) const = 0;
-
   /* Return true if it safe to discard the given state (to help
      when simplifying state objects).
      States that need leak detection should return false.  */
   virtual bool can_purge_p (state_t s) const = 0;
 
+  /* Called when VAR leaks (and !can_purge_p).  */
+  virtual pending_diagnostic *on_leak (tree var ATTRIBUTE_UNUSED) const
+  {
+    return NULL;
+  }
+
   void validate (state_t s) const;
 
 protected:
diff --git a/gcc/testsuite/gcc.dg/analyzer/setjmp-7.c b/gcc/testsuite/gcc.dg/analyzer/setjmp-7.c
index 2d44926..ee4183d 100644
--- a/gcc/testsuite/gcc.dg/analyzer/setjmp-7.c
+++ b/gcc/testsuite/gcc.dg/analyzer/setjmp-7.c
@@ -8,12 +8,12 @@  static jmp_buf env;
 
 static void inner (void)
 {
-  longjmp (env, 1); /* { dg-warning "leak of 'ptr'" "" { xfail *-*-* } } */
+  longjmp (env, 1); /* { dg-warning "leak of 'ptr'" } */
 }
 
 static void middle (void)
 {
-  void *ptr = malloc (1024); /* { dg-message "allocated here"  "" { xfail *-*-* } }  */
+  void *ptr = malloc (1024); /* { dg-message "allocated here" }  */
   inner ();
   free (ptr);
 }
diff --git a/gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c b/gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c
new file mode 100644
index 0000000..1d5fc2e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/setjmp-7a.c
@@ -0,0 +1,101 @@ 
+/* { dg-additional-options "-fdiagnostics-show-line-numbers -fdiagnostics-nn-line-numbers -fdiagnostics-path-format=inline-events -fdiagnostics-show-caret" } */
+
+#include <setjmp.h>
+#include <stdlib.h>
+
+extern void foo (int);
+
+static jmp_buf env;
+
+static void inner (void)
+{
+  longjmp (env, 1); /* { dg-warning "leak of 'ptr'" } */
+}
+
+static void middle (void)
+{
+  void *ptr = malloc (1024);
+  inner ();
+  free (ptr);
+}
+
+void outer (void)
+{
+  int i;
+
+  foo (0);
+
+  i = setjmp(env);
+
+  if (i == 0)
+    {
+      foo (1);
+      middle ();
+    }
+
+  foo (3);
+}
+
+/* { dg-begin-multiline-output "" }
+   NN |   longjmp (env, 1);
+      |   ^~~~~~~~~~~~~~~~
+  'outer': event 1
+    |
+    |   NN | void outer (void)
+    |      |      ^~~~~
+    |      |      |
+    |      |      (1) entry to 'outer'
+    |
+  'outer': event 2
+    |
+    |   NN |   i = setjmp(env);
+    |      |       ^~~~~~
+    |      |       |
+    |      |       (2) 'setjmp' called here
+    |
+  'outer': events 3-5
+    |
+    |   NN |   if (i == 0)
+    |      |      ^
+    |      |      |
+    |      |      (3) following 'true' branch (when 'i == 0')...
+    |   NN |     {
+    |   NN |       foo (1);
+    |      |       ~~~~~~~
+    |      |       |
+    |      |       (4) ...to here
+    |   NN |       middle ();
+    |      |       ~~~~~~~~~
+    |      |       |
+    |      |       (5) calling 'middle' from 'outer'
+    |
+    +--> 'middle': events 6-8
+           |
+           |   NN | static void middle (void)
+           |      |             ^~~~~~
+           |      |             |
+           |      |             (6) entry to 'middle'
+           |   NN | {
+           |   NN |   void *ptr = malloc (1024);
+           |      |               ~~~~~~~~~~~~~
+           |      |               |
+           |      |               (7) allocated here
+           |   NN |   inner ();
+           |      |   ~~~~~~~~   
+           |      |   |
+           |      |   (8) calling 'inner' from 'middle'
+           |
+           +--> 'inner': events 9-10
+                  |
+                  |   NN | static void inner (void)
+                  |      |             ^~~~~
+                  |      |             |
+                  |      |             (9) entry to 'inner'
+                  |   NN | {
+                  |   NN |   longjmp (env, 1);
+                  |      |   ~~~~~~~~~~~~~~~~
+                  |      |   |
+                  |      |   (10) 'ptr' leaks here; was allocated at (7)
+                  |
+    { dg-end-multiline-output "" } */
+// TODO: show the rewind to the setjmp