[PATCH] constrain PHI handling in -Wuse-after-free (PR104232)

[PATCH] constrain PHI handling in -Wuse-after-free (PR104232)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

The indiscriminate PHI handling by -Wuse-after-free has caused
the false positive reported in PR 104232.  The attached patch
refines the handling to only consider PHIs all of whose operands
refer to the same object and disregard the rest.

Tested on x86_64-linux and by compiling a few toolchain projects,
including Glibc and Binutils/GDB, to verify the absence of false
positives.

Martin

[-- Attachment #2: gcc-104232.diff --]
[-- Type: text/x-patch, Size: 6907 bytes --]

Constrain PHI handling in -Wuse-after-free [PR104232].

Resolves:
PR middle-end/104232 - spurious -Wuse-after-free after conditional free

gcc/ChangeLog:

	PR middle-end/104232
	* gimple-ssa-warn-access.cc (pointers_related_p): Add argument.
	Handle PHIs.  Add a synonymous overload.
	(pass_waccess::check_pointer_uses): Call pointers_related_p.

gcc/testsuite/ChangeLog:

	PR middle-end/104232
	* g++.dg/warn/Wuse-after-free4.C: New test.
	* gcc.dg/Wuse-after-free-2.c: New test.
	* gcc.dg/Wuse-after-free-3.c: New test.

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 3dcaf4230b8..ad5e2f4458e 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4080,7 +4080,8 @@ maybe_warn_mismatched_realloc (tree ptr, gimple *realloc_stmt, gimple *stmt)
    either don't or their relationship cannot be determined.  */
 
 static bool
-pointers_related_p (gimple *stmt, tree p, tree q, pointer_query &qry)
+pointers_related_p (gimple *stmt, tree p, tree q, pointer_query &qry,
+		    auto_bitmap &visited)
 {
   if (!ptr_derefs_may_alias_p (p, q))
     return false;
@@ -4093,7 +4094,47 @@ pointers_related_p (gimple *stmt, tree p, tree q, pointer_query &qry)
        it involves a self-referential PHI.  Return a conservative result.  */
     return false;
 
-  return pref.ref == qref.ref;
+  if (pref.ref == qref.ref)
+    return true;
+
+  /* If either pointer is a PHI, iterate over all its operands and
+     return true if they're all related to the other pointer.  */
+  tree ptr = q;
+  unsigned version;
+  gphi *phi = pref.phi ();
+  if (phi)
+    version = SSA_NAME_VERSION (pref.ref);
+  else
+    {
+      phi = qref.phi ();
+      if (!phi)
+	return false;
+
+      ptr = p;
+      version = SSA_NAME_VERSION (qref.ref);
+    }
+
+  if (!bitmap_set_bit (visited, version))
+    return true;
+
+  unsigned nargs = gimple_phi_num_args (phi);
+  for (unsigned i = 0; i != nargs; ++i)
+    {
+      tree arg = gimple_phi_arg_def (phi, i);
+      if (!pointers_related_p (stmt, arg, ptr, qry, visited))
+	return false;
+    }
+
+  return true;
+}
+
+/* Convenience wrapper for the above.  */
+
+static bool
+pointers_related_p (gimple *stmt, tree p, tree q, pointer_query &qry)
+{
+  auto_bitmap visited;
+  return pointers_related_p (stmt, p, q, qry, visited);
 }
 
 /* For a STMT either a call to a deallocation function or a clobber, warn
@@ -4192,7 +4233,12 @@ pass_waccess::check_pointer_uses (gimple *stmt, tree ptr,
 	    {
 	      if (gimple_code (use_stmt) == GIMPLE_PHI)
 		{
+		  /* Only add a PHI result to POINTERS if all its
+		     operands are related to PTR, otherwise continue.  */
 		  tree lhs = gimple_phi_result (use_stmt);
+		  if (!pointers_related_p (stmt, lhs, ptr, m_ptr_qry))
+		    continue;
+
 		  if (TREE_CODE (lhs) == SSA_NAME)
 		    {
 		      pointers.safe_push (lhs);
diff --git a/gcc/testsuite/g++.dg/warn/Wuse-after-free4.C b/gcc/testsuite/g++.dg/warn/Wuse-after-free4.C
new file mode 100644
index 00000000000..599d9bfe3c7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wuse-after-free4.C
@@ -0,0 +1,27 @@
+/* PR middle-end/104232 - spurious -Wuse-after-free after conditional free
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+char* f (void);
+
+struct A
+{
+  char *p;
+  A (): p () { }
+  ~A ()
+  {
+    __builtin_free (p);           // { dg-bogus "-Wuse-after-free" }
+  }
+};
+
+int test_no_warn (void)
+{
+  A px, qx;
+
+  qx.p = f ();
+  if (!qx.p)
+    return 0;
+
+  px.p = f ();
+  return 1;
+}
diff --git a/gcc/testsuite/gcc.dg/Wuse-after-free-2.c b/gcc/testsuite/gcc.dg/Wuse-after-free-2.c
new file mode 100644
index 00000000000..9f7ed4529f0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wuse-after-free-2.c
@@ -0,0 +1,115 @@
+/* PR middle-end/104232 - spurious -Wuse-after-free after conditional free
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+void free (void*);
+
+void sink (void*);
+
+void nowarn_cond_2 (char *p0, char *q0, int i)
+{
+  char *r = i ? p0 : q0;
+
+  free (p0);
+
+  /* The use of a PHI operand could be diagnosed using the "maybe" form
+     of the warning at level 2 but it's not done.  If it ever changes
+     this test and those below will need to be updated.  */
+  sink (r);
+}
+
+void nowarn_cond_2_null (char *p0, int i)
+{
+  char *r = i ? p0 : 0;
+
+  free (p0);
+  sink (r);
+}
+
+void nowarn_cond_3 (char *p0, char *q0, int i)
+{
+  char *r = i < 0 ? p0 - 1 : 0 < i ? p0 + 1 : q0;
+
+  free (p0);
+  sink (r);
+}
+
+void nowarn_cond_3_null (char *p0, int i)
+{
+  char *r = i < 0 ? p0 - 1 : 0 < i ? p0 + 1 : 0;
+
+  free (p0);
+  sink (r);
+}
+
+void nowarn_cond_4 (char *p0, char *q0, int i)
+{
+  char *r = i < -1 ? p0 - 2 : i < 0 ? p0 - 1 : 1 < i ? p0 + 1 : q0;
+
+  free (p0);
+  sink (r);
+}
+
+int nowarn_cond_loop (char *p)
+{
+  char *q = p;
+  while (*q)
+    {
+      if (*q == 'x')
+        {
+          q = "";
+          break;
+        }
+      ++q;
+    }
+
+  free (p);
+  return *q;
+}
+
+
+void warn_cond_2_cst (char *p, int i)
+{
+  /* Same as nowarn_cond_2() above but with R being derived only from
+     P, which means that any R's use after P has been freed should be
+     diagnosed.  */
+  char *r = i ? p + 1 : p + 2;
+
+  free (p);         // { dg-message "call to 'free'" }
+  sink (r);         // { dg-warning "pointer used after 'free'" }
+}
+
+void warn_cond_2_var (char *p, int i, int j)
+{
+  char *r = i ? p + i : p + j;
+
+  free (p);         // { dg-message "call to 'free'" }
+  sink (r);         // { dg-warning "pointer used after 'free'" }
+}
+
+void warn_cond_3_var (char *p0, int i, int j)
+{
+  char *r = i < 0 ? p0 - i : 0 < i ? p0 + j : p0 + i + j;
+
+  free (p0);        // { dg-message "call to 'free'" }
+  sink (r + 1);     // { dg-warning "pointer used after 'free'" }
+}
+
+int warn_cond_4 (char *p0, char *q0, int i)
+{
+  char *r = i < -1 ? p0 - 2 : i < 0 ? p0 - 1 : 1 < i ? p0 + 2 : p0 + 1;
+
+  free (p0);        // { dg-message "call to 'free'" }
+  return *r;        // { dg-warning "pointer used after 'free'" }
+}
+
+int warn_cond_loop (char *p)
+{
+  char *q = p;
+
+  while (*q)
+    ++q;
+
+  free (p);         // { dg-message "call to 'free'" }
+  return *q;        // { dg-warning "pointer 'q' used after 'free'" }
+}
diff --git a/gcc/testsuite/gcc.dg/Wuse-after-free-3.c b/gcc/testsuite/gcc.dg/Wuse-after-free-3.c
new file mode 100644
index 00000000000..d1bcfcb3dda
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wuse-after-free-3.c
@@ -0,0 +1,22 @@
+/* PR middle-end/104232 - spurious -Wuse-after-free after conditional free
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+char* f (void);
+
+static inline void freep (void *p)
+{
+  __builtin_free (*(void**)p);    // { dg-bogus "-Wuse-after-free" }
+}
+
+int test_no_warn (void)
+{
+  __attribute__ ((__cleanup__ (freep))) char *s = 0, *t = 0;
+
+  t = f ();
+  if (!t)
+    return 0;
+
+  s = f ();
+  return 1;
+}

Re: [PATCH] constrain PHI handling in -Wuse-after-free (PR104232)

[Richard Biener]

On Thu, Jan 27, 2022 at 7:58 PM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The indiscriminate PHI handling by -Wuse-after-free has caused
> the false positive reported in PR 104232.  The attached patch
> refines the handling to only consider PHIs all of whose operands
> refer to the same object and disregard the rest.
>
> Tested on x86_64-linux and by compiling a few toolchain projects,
> including Glibc and Binutils/GDB, to verify the absence of false
> positives.

OK.

> Martin