This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH 07/11] [analyzer] Fix missing leak on longjmp past a free
- From: David Malcolm <dmalcolm at redhat dot com>
- To: gcc-patches at gcc dot gnu dot org
- Cc: David Malcolm <dmalcolm at redhat dot com>
- Date: Wed, 20 Nov 2019 16:13:46 -0500
- Subject: [PATCH 07/11] [analyzer] Fix missing leak on longjmp past a free
- References: <1574284430-8776-1-git-send-email-dmalcolm@redhat.com>
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
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
--
1.8.5.3