Whilst analyzing the reproducer for detecting CVE-2005-1689
(krb5-1.4.1's src/lib/krb5/krb/recvauth.c), the analyzer reported
11 double-free diagnostics on lines of the form:
krb5_xfree(inbuf.data);
with no deduplication occcurring.
The root cause is that the diagnostics each have a COMPONENT_REF for
the inbuf.data, but they are different trees, and the de-duplication
logic was using pointer equality.
This patch replaces the pointer equality tests with calls to a new
pending_diagnostic::same_tree_p, implemented using simple_cst_equal.
With this patch, de-duplication occurs, and only 3 diagnostics are
reported. The 11 diagnostics are partitioned into 3 dedupe keys,
2 with 2 duplicates and 1 with 7 duplicates.
gcc/analyzer/ChangeLog:
* diagnostic-manager.cc (saved_diagnostic::operator==): Move here
from header. Replace pointer equality test on m_var with call to
pending_diagnostic::same_tree_p.
* diagnostic-manager.h (saved_diagnostic::operator==): Move to
diagnostic-manager.cc.
* pending-diagnostic.cc (pending_diagnostic::same_tree_p): New.
* pending-diagnostic.h (pending_diagnostic::same_tree_p): New.
* sm-file.cc (file_diagnostic::subclass_equal_p): Replace pointer
equality on m_arg with call to pending_diagnostic::same_tree_p.
* sm-malloc.cc (malloc_diagnostic::subclass_equal_p): Likewise.
(possible_null_arg::subclass_equal_p): Likewise.
(null_arg::subclass_equal_p): Likewise.
(free_of_non_heap::subclass_equal_p): Likewise.
* sm-pattern-test.cc (pattern_match::operator==): Likewise.
* sm-sensitive.cc (exposure_through_output_file::operator==):
Likewise.
* sm-taint.cc (tainted_array_index::operator==): Likewise.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/CVE-2005-1689-dedupe-issue.c: New test.
---
gcc/analyzer/diagnostic-manager.cc | 14 ++++++++++
gcc/analyzer/diagnostic-manager.h | 13 +---------
gcc/analyzer/pending-diagnostic.cc | 9 +++++++
gcc/analyzer/pending-diagnostic.h | 4 +++
gcc/analyzer/sm-file.cc | 2 +-
gcc/analyzer/sm-malloc.cc | 8 +++---
gcc/analyzer/sm-pattern-test.cc | 4 +--
gcc/analyzer/sm-sensitive.cc | 2 +-
gcc/analyzer/sm-taint.cc | 2 +-
.../analyzer/CVE-2005-1689-dedupe-issue.c | 26 +++++++++++++++++++
10 files changed, 63 insertions(+), 21 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/CVE-2005-1689-dedupe-issue.c
diff --git a/gcc/analyzer/diagnostic-manager.cc
b/gcc/analyzer/diagnostic-manager.cc
index 47d32f4f4c6c..5e4e44ecae73 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -65,6 +65,20 @@ saved_diagnostic::~saved_diagnostic ()
delete m_d;
}
+bool
+saved_diagnostic::operator== (const saved_diagnostic &other) const
+{
+ return (m_sm == other.m_sm
+ /* We don't compare m_enode. */
+ && m_snode == other.m_snode
+ && m_stmt == other.m_stmt
+ /* We don't compare m_stmt_finder. */
+ && pending_diagnostic::same_tree_p (m_var, other.m_var)
+ && m_state == other.m_state
+ && m_d->equal_p (*other.m_d)
+ && m_trailing_eedge == other.m_trailing_eedge);
+}
+
/* class diagnostic_manager. */
/* diagnostic_manager's ctor. */
diff --git a/gcc/analyzer/diagnostic-manager.h
b/gcc/analyzer/diagnostic-manager.h
index 5490a2f4e233..a8f9eb898ba7 100644
--- a/gcc/analyzer/diagnostic-manager.h
+++ b/gcc/analyzer/diagnostic-manager.h
@@ -36,18 +36,7 @@ public:
pending_diagnostic *d);
~saved_diagnostic ();
- bool operator== (const saved_diagnostic &other) const
- {
- return (m_sm == other.m_sm
- /* We don't compare m_enode. */
- && m_snode == other.m_snode
- && m_stmt == other.m_stmt
- /* We don't compare m_stmt_finder. */
- && m_var == other.m_var
- && m_state == other.m_state
- && m_d->equal_p (*other.m_d)
- && m_trailing_eedge == other.m_trailing_eedge);
- }
+ bool operator== (const saved_diagnostic &other) const;
//private:
const state_machine *m_sm;
diff --git a/gcc/analyzer/pending-diagnostic.cc
b/gcc/analyzer/pending-diagnostic.cc
index ae61b47bef66..29fcd7dc7e0d 100644
--- a/gcc/analyzer/pending-diagnostic.cc
+++ b/gcc/analyzer/pending-diagnostic.cc
@@ -61,4 +61,13 @@ evdesc::event_desc::formatted_print (const char *fmt, ...)
const
return result;
}
+/* Return true if T1 and T2 are "the same" for the purposes of
+ diagnostic deduplication. */
+
+bool
+pending_diagnostic::same_tree_p (tree t1, tree t2)
+{
+ return simple_cst_equal (t1, t2) == 1;
+}
+
#endif /* #if ENABLE_ANALYZER */
diff --git a/gcc/analyzer/pending-diagnostic.h
b/gcc/analyzer/pending-diagnostic.h
index 15a1379e8fd1..494a6852d91a 100644
--- a/gcc/analyzer/pending-diagnostic.h
+++ b/gcc/analyzer/pending-diagnostic.h
@@ -172,6 +172,10 @@ class pending_diagnostic
below for a convenience subclass for implementing this. */
virtual bool subclass_equal_p (const pending_diagnostic &other) const = 0;
+ /* Return true if T1 and T2 are "the same" for the purposes of
+ diagnostic deduplication. */
+ static bool same_tree_p (tree t1, tree t2);
+
/* For greatest precision-of-wording, the various following "describe_*"
virtual functions give the pending diagnostic a way to describe events
in a diagnostic_path in terms that make sense for that diagnostic.
diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc
index 20963c3e59b4..e4eeb1f41be9 100644
--- a/gcc/analyzer/sm-file.cc
+++ b/gcc/analyzer/sm-file.cc
@@ -91,7 +91,7 @@ public:
bool subclass_equal_p (const pending_diagnostic &base_other) const OVERRIDE
{
- return m_arg == ((const file_diagnostic &)base_other).m_arg;
+ return same_tree_p (m_arg, ((const file_diagnostic &)base_other).m_arg);
}
label_text describe_state_change (const evdesc::state_change &change)
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index badae51e1d75..73a6c3d9294c 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -99,7 +99,7 @@ public:
bool subclass_equal_p (const pending_diagnostic &base_other) const OVERRIDE
{
- return m_arg == ((const malloc_diagnostic &)base_other).m_arg;
+ return same_tree_p (m_arg, ((const malloc_diagnostic &)base_other).m_arg);
}
label_text describe_state_change (const evdesc::state_change &change)
@@ -279,7 +279,7 @@ public:
{
const possible_null_arg &sub_other
= (const possible_null_arg &)base_other;
- return (m_arg == sub_other.m_arg
+ return (same_tree_p (m_arg, sub_other.m_arg)
&& m_fndecl == sub_other.m_fndecl
&& m_arg_idx == sub_other.m_arg_idx);
}
@@ -370,7 +370,7 @@ public:
{
const null_arg &sub_other
= (const null_arg &)base_other;
- return (m_arg == sub_other.m_arg
+ return (same_tree_p (m_arg, sub_other.m_arg)
&& m_fndecl == sub_other.m_fndecl
&& m_arg_idx == sub_other.m_arg_idx);
}
@@ -496,7 +496,7 @@ public:
FINAL OVERRIDE
{
const free_of_non_heap &other = (const free_of_non_heap &)base_other;
- return (m_arg == other.m_arg && m_kind == other.m_kind);
+ return (same_tree_p (m_arg, other.m_arg) && m_kind == other.m_kind);
}
bool emit (rich_location *rich_loc) FINAL OVERRIDE
diff --git a/gcc/analyzer/sm-pattern-test.cc b/gcc/analyzer/sm-pattern-test.cc
index f7fc21f04049..6ae89284e97c 100644
--- a/gcc/analyzer/sm-pattern-test.cc
+++ b/gcc/analyzer/sm-pattern-test.cc
@@ -75,9 +75,9 @@ public:
bool operator== (const pattern_match &other) const
{
- return (m_lhs == other.m_lhs
+ return (same_tree_p (m_lhs, other.m_lhs)
&& m_op == other.m_op
- && m_rhs == other.m_rhs);
+ && same_tree_p (m_rhs, other.m_rhs));
}
bool emit (rich_location *rich_loc) FINAL OVERRIDE
diff --git a/gcc/analyzer/sm-sensitive.cc b/gcc/analyzer/sm-sensitive.cc
index 37219b915da3..09745504a81d 100644
--- a/gcc/analyzer/sm-sensitive.cc
+++ b/gcc/analyzer/sm-sensitive.cc
@@ -92,7 +92,7 @@ public:
bool operator== (const exposure_through_output_file &other) const
{
- return m_arg == other.m_arg;
+ return same_tree_p (m_arg, other.m_arg);
}
bool emit (rich_location *rich_loc) FINAL OVERRIDE
diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index 8b20f78ffe60..aa25a2da0577 100644
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -97,7 +97,7 @@ public:
bool operator== (const tainted_array_index &other) const
{
- return m_arg == other.m_arg;
+ return same_tree_p (m_arg, other.m_arg);
}
bool emit (rich_location *rich_loc) FINAL OVERRIDE
diff --git a/gcc/testsuite/gcc.dg/analyzer/CVE-2005-1689-dedupe-issue.c
b/gcc/testsuite/gcc.dg/analyzer/CVE-2005-1689-dedupe-issue.c
new file mode 100644
index 000000000000..941d3b834a1f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/CVE-2005-1689-dedupe-issue.c
@@ -0,0 +1,26 @@
+#include <stdlib.h>
+
+typedef struct _krb5_data {
+ char *data;
+} krb5_data;
+
+/* Ensure that we de-duplicate the various paths to reach here,
+ and only emit one diagnostic. */
+
+void
+recvauth_common(krb5_data inbuf)
+{
+ free(inbuf.data);
+ free(inbuf.data); /* { dg-warning "double-'free'" } */
+ /* { dg-message "2 duplicates" "" { target *-*-* } .-1 } */
+}
+
+void krb5_recvauth(krb5_data inbuf)
+{
+ recvauth_common(inbuf);
+}
+
+void krb5_recvauth_version(krb5_data inbuf)
+{
+ recvauth_common(inbuf);
+}
--
2.21.0
