analyzer: fix deref-before-check false +ves seen in haproxy [PR108475,PR109060]

Integration testing showed various false positives from
-Wanalyzer-deref-before-check where the expression that's dereferenced
is different from the one that's checked, but the diagnostic is emitted
because they both evaluate to the same symbolic value.

This patch rejects such warnings, unless we have tree expressions for
both and that both tree expressions are "spelled the same way" i.e.
would be printed to the same user-facing string.

gcc/analyzer/ChangeLog:
	PR analyzer/108475
	PR analyzer/109060
	* sm-malloc.cc (deref_before_check::deref_before_check):
	Initialize new field m_deref_expr.  Assert that arg is non-NULL.
	(deref_before_check::emit): Reject cases where the spelling of the
	thing that was dereferenced differs from that of what is checked,
	or if the dereference expression was not found.  Remove code to
	handle NULL m_arg.
	(deref_before_check::describe_state_change): Remove code to handle
	NULL m_arg.
	(deref_before_check::describe_final_event): Likewise.
	(deref_before_check::sufficiently_similar_p): New.
	(deref_before_check::m_deref_expr): New field.
	(malloc_state_machine::maybe_complain_about_deref_before_check):
	Don't warn if the diag_ptr is NULL.

gcc/testsuite/ChangeLog:
	PR analyzer/108475
	PR analyzer/109060
	* gcc.dg/analyzer/deref-before-check-pr108475-1.c: New test.
	* gcc.dg/analyzer/deref-before-check-pr108475-haproxy-tcpcheck.c:
	New test.
	* gcc.dg/analyzer/deref-before-check-pr109060-haproxy-cfgparse.c:
	New test.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>

1 files changed, 44 insertions, 37 deletions

diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index 1ea9b30..16883d3 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -1498,8 +1498,11 @@ public:
   deref_before_check (const malloc_state_machine &sm, tree arg)
   : malloc_diagnostic (sm, arg),
     m_deref_enode (NULL),
+    m_deref_expr (NULL),
     m_check_enode (NULL)
-  {}
+  {
+    gcc_assert (arg);
+  }
 
   const char *get_kind () const final override { return "deref_before_check"; }
 
@@ -1560,6 +1563,15 @@ public:
     if (linemap_location_from_macro_definition_p (line_table, check_loc))
       return false;
 
+    /* Reject if m_deref_expr is sufficiently different from m_arg
+       for cases where the dereference is spelled differently from
+       the check, which is probably two different ways to get the
+       same svalue, and thus not worth reporting.  */
+    if (!m_deref_expr)
+      return false;
+    if (!sufficiently_similar_p (m_deref_expr, m_arg))
+      return false;
+
     /* Reject the warning if the deref's BB doesn't dominate that
        of the check, so that we don't warn e.g. for shared cleanup
        code that checks a pointer for NULL, when that code is sometimes
@@ -1572,15 +1584,10 @@ public:
 			 m_deref_enode->get_supernode ()->m_bb))
       return false;
 
-    if (m_arg)
-      return warning_at (rich_loc, get_controlling_option (),
-			 "check of %qE for NULL after already"
-			 " dereferencing it",
-			 m_arg);
-    else
-      return warning_at (rich_loc, get_controlling_option (),
-			 "check of pointer for NULL after already"
-			 " dereferencing it");
+    return warning_at (rich_loc, get_controlling_option (),
+		       "check of %qE for NULL after already"
+		       " dereferencing it",
+		       m_arg);
   }
 
   label_text describe_state_change (const evdesc::state_change &change)
@@ -1591,11 +1598,9 @@ public:
       {
 	m_first_deref_event = change.m_event_id;
 	m_deref_enode = change.m_event.get_exploded_node ();
-	if (m_arg)
-	  return change.formatted_print ("pointer %qE is dereferenced here",
-					 m_arg);
-	else
-	  return label_text::borrow ("pointer is dereferenced here");
+	m_deref_expr = change.m_expr;
+	return change.formatted_print ("pointer %qE is dereferenced here",
+				       m_arg);
       }
     return malloc_diagnostic::describe_state_change (change);
   }
@@ -1604,31 +1609,32 @@ public:
   {
     m_check_enode = ev.m_event.get_exploded_node ();
     if (m_first_deref_event.known_p ())
-      {
-	if (m_arg)
-	  return ev.formatted_print ("pointer %qE is checked for NULL here but"
-				     " it was already dereferenced at %@",
-				     m_arg, &m_first_deref_event);
-	else
-	  return ev.formatted_print ("pointer is checked for NULL here but"
-				     " it was already dereferenced at %@",
-				     &m_first_deref_event);
-      }
+      return ev.formatted_print ("pointer %qE is checked for NULL here but"
+				 " it was already dereferenced at %@",
+				 m_arg, &m_first_deref_event);
     else
-      {
-	if (m_arg)
-	  return ev.formatted_print ("pointer %qE is checked for NULL here but"
-				     " it was already dereferenced",
-				     m_arg);
-	else
-	  return ev.formatted_print ("pointer is checked for NULL here but"
-				     " it was already dereferenced");
-      }
+      return ev.formatted_print ("pointer %qE is checked for NULL here but"
+				 " it was already dereferenced",
+				 m_arg);
   }
 
 private:
+  static bool sufficiently_similar_p (tree expr_a, tree expr_b)
+  {
+    pretty_printer *pp_a = global_dc->printer->clone ();
+    pretty_printer *pp_b = global_dc->printer->clone ();
+    pp_printf (pp_a, "%qE", expr_a);
+    pp_printf (pp_b, "%qE", expr_b);
+    bool result = (strcmp (pp_formatted_text (pp_a), pp_formatted_text (pp_b))
+		   == 0);
+    delete pp_a;
+    delete pp_b;
+    return result;
+  }
+
   diagnostic_event_id_t m_first_deref_event;
   const exploded_node *m_deref_enode;
+  tree m_deref_expr;
   const exploded_node *m_check_enode;
 };
 
@@ -2141,9 +2147,10 @@ maybe_complain_about_deref_before_check (sm_context *sm_ctxt,
     return;
 
   tree diag_ptr = sm_ctxt->get_diagnostic_tree (ptr);
-  sm_ctxt->warn
-    (node, stmt, ptr,
-     make_unique<deref_before_check> (*this, diag_ptr));
+  if (diag_ptr)
+    sm_ctxt->warn
+      (node, stmt, ptr,
+       make_unique<deref_before_check> (*this, diag_ptr));
   sm_ctxt->set_next_state (stmt, ptr, m_stop);
 }