analyzer: fix dedupe issue seen with CVE-2005-1689

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.

diff --git a/gcc/analyzer/ChangeLog b/gcc/analyzer/ChangeLog
index 4fe354a0c975944c0fe9b44370dfe7a92d747bd3..9afb288266d9187ae407f499c7e00ed7eddc36f3 100644 (file)
--- a/gcc/analyzer/ChangeLog
+++ b/gcc/analyzer/ChangeLog
@@ -1,3 +1,23 @@
+2020-01-14  David Malcolm  <dmalcolm@redhat.com>
+
+       * 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.
+
 2020-01-14  David Malcolm  <dmalcolm@redhat.com>
 
        * diagnostic-manager.cc (dedupe_winners::add): Add logging

diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc
index 7bd21d63e3888a1c914ad0a9df369819706c6fb3..ea2ff30b9a0f6308890868aa9a9f8e209439c5f2 100644 (file)
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -91,6 +91,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 aa939430348fe628af8cdcf28a88509c9ccae0bd..c0f13bf8895913a252581446a81dc898f733f8c2 100644 (file)
--- a/gcc/analyzer/diagnostic-manager.h
+++ b/gcc/analyzer/diagnostic-manager.h
@@ -34,18 +34,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 f6c48837a59bf974c032d3bae910d9af50f5e7c6..b692c98304f46aa0cba03ac81fd6e73a3927ebb9 100644 (file)
--- a/gcc/analyzer/pending-diagnostic.cc
+++ b/gcc/analyzer/pending-diagnostic.cc
@@ -67,4 +67,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 bb03e752530d5ac04cbfb0addfb3127fccbc8a39..6132fcfa481c06327d0530571f647c43ff49c039 100644 (file)
--- a/gcc/analyzer/pending-diagnostic.h
+++ b/gcc/analyzer/pending-diagnostic.h
@@ -169,6 +169,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 ba18bf71a73da002afa52ec140aaf3f9c85aea8a..375f5220246e0041b3955def0f730d8e56fcbc80 100644 (file)
--- a/gcc/analyzer/sm-file.cc
+++ b/gcc/analyzer/sm-file.cc
@@ -96,7 +96,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 80ceb0f59137eb4b7716b6d08cbfa3470bc11069..15fa1c764718ab4929d89345264bfbd4187f5dae 100644 (file)
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -102,7 +102,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)
@@ -282,7 +282,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);
   }
@@ -373,7 +373,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);
   }
@@ -499,7 +499,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 8c70858e938b86d968c03b76caf8a1c21ff043bf..571e13eb47cde53491028eb1f4facaeaf9908ab3 100644 (file)
--- a/gcc/analyzer/sm-pattern-test.cc
+++ b/gcc/analyzer/sm-pattern-test.cc
@@ -78,9 +78,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 efb24325fbd19d96d95a653f855f70c949e21d4f..16b53e2c9e4ba93f67669fdfaae22470ecb4a66c 100644 (file)
--- a/gcc/analyzer/sm-sensitive.cc
+++ b/gcc/analyzer/sm-sensitive.cc
@@ -95,7 +95,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 9382853503f49eb6341423312104ef8eb32e7bcc..e454407e7af3a8dca387d24a7fe18b83b0919b1c 100644 (file)
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -100,7 +100,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/ChangeLog b/gcc/testsuite/ChangeLog
index 8e99328feb2e848e9b98363b53c6006b5763d491..1cdaa810d77a6c4cbccffd283839d9ceb57fe01d 100644 (file)
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2020-01-14  David Malcolm  <dmalcolm@redhat.com>
+
+       * gcc.dg/analyzer/CVE-2005-1689-dedupe-issue.c: New test.
+
 2020-01-15  Jakub Jelinek  <jakub@redhat.com>
 
        PR lto/91576

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 (file)
index 0000000..941d3b8
--- /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);
+}