[PR,middle-end/78548] fix latent double free in tree-ssa-uninit.c
diff mbox

Message ID be4a21f7-998e-9922-e220-a27e1eca03aa@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Dec. 1, 2016, 11:03 a.m. UTC
This looks like a latent problem in the -Wmaybe-uninitialized code 
unrelated to my work.

The problem here is a sequence of simplify_preds() followed by 
normalize_preds() that I added, but is based on prior art all over the file:

+      simplify_preds (&uninit_preds, NULL, false);
+      uninit_preds = normalize_preds (uninit_preds, NULL, false);

The problem is that in this particular testcase we trigger 
simplify_preds_4 which makes a copy of a chain, frees the chain, and 
then tries to use the chain later (in normalize_preds).  The 
normalize_preds() function tries to free again the chain and we blow up:

This is the main problem in simplify_preds_4:

   /* Now clean up the chain.  */
   if (simplified)
     {
       for (i = 0; i < n; i++)
	{
	  if ((*preds)[i].is_empty ())
	    continue;
	  s_preds.safe_push ((*preds)[i]);
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// Makes a copy of the pred_chain.
	}

       destroy_predicate_vecs (preds);
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// free() all the pred_chain's.

       (*preds) = s_preds;
// ^^^^^^^^^^^^^^^^^^^^^^
// Wait a minute, we still keep a copy of the pred_chains.
       s_preds = vNULL;
     }

I have no idea how this worked even before my patch.  Perhaps we never 
had a simplify_preds() followed by a normalize_preds() where the 
simplification involved a call to simplify_preds_4.

Interestingly enough, simplify_preds_2() has the exact same code, but 
with the fix I am proposing. So this seems like an oversight.  Also, the 
fact that the simplification in simplify_preds_2 is more common than the 
simplification performed in simplify_preds_4 would suggest that 
simplify_preds_4 was uncommon enough and probably wasn't been used in a 
simplify_preds/normalize_preds combo.

Anyways... I've made some other cleanups to the code, but the main gist 
of the entire patch is:

-      destroy_predicate_vecs (preds);
+      preds->release ();

That is, release preds, but don't free the associated memory with the 
pred_chain's therein.

This patch is on top of my pending patch here:

	https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02900.html

Tested on x86-64 Linux.

OK for trunk?
Aldy
commit ec4443b8dcf89465cb8d9735a3e0a27b181c975c
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Thu Dec 1 04:53:38 2016 -0500

            PR middle-end/78548
            * tree-ssa-uninit.c (simplify_preds_4): Call release() instead of
            destroy_predicate_vecs.
            (uninit_uses_cannot_happen): Make uninit_preds a scalar.

Comments

Richard Biener Dec. 1, 2016, 12:32 p.m. UTC | #1
On Thu, Dec 1, 2016 at 12:03 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> This looks like a latent problem in the -Wmaybe-uninitialized code unrelated
> to my work.
>
> The problem here is a sequence of simplify_preds() followed by
> normalize_preds() that I added, but is based on prior art all over the file:
>
> +      simplify_preds (&uninit_preds, NULL, false);
> +      uninit_preds = normalize_preds (uninit_preds, NULL, false);
>
> The problem is that in this particular testcase we trigger simplify_preds_4
> which makes a copy of a chain, frees the chain, and then tries to use the
> chain later (in normalize_preds).  The normalize_preds() function tries to
> free again the chain and we blow up:
>
> This is the main problem in simplify_preds_4:
>
>   /* Now clean up the chain.  */
>   if (simplified)
>     {
>       for (i = 0; i < n; i++)
>         {
>           if ((*preds)[i].is_empty ())
>             continue;
>           s_preds.safe_push ((*preds)[i]);
> // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> // Makes a copy of the pred_chain.
>         }
>
>       destroy_predicate_vecs (preds);
> // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> // free() all the pred_chain's.
>
>       (*preds) = s_preds;
> // ^^^^^^^^^^^^^^^^^^^^^^
> // Wait a minute, we still keep a copy of the pred_chains.
>       s_preds = vNULL;
>     }
>
> I have no idea how this worked even before my patch.  Perhaps we never had a
> simplify_preds() followed by a normalize_preds() where the simplification
> involved a call to simplify_preds_4.
>
> Interestingly enough, simplify_preds_2() has the exact same code, but with
> the fix I am proposing. So this seems like an oversight.  Also, the fact
> that the simplification in simplify_preds_2 is more common than the
> simplification performed in simplify_preds_4 would suggest that
> simplify_preds_4 was uncommon enough and probably wasn't been used in a
> simplify_preds/normalize_preds combo.
>
> Anyways... I've made some other cleanups to the code, but the main gist of
> the entire patch is:
>
> -      destroy_predicate_vecs (preds);
> +      preds->release ();
>
> That is, release preds, but don't free the associated memory with the
> pred_chain's therein.
>
> This patch is on top of my pending patch here:
>
>         https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02900.html
>
> Tested on x86-64 Linux.
>
> OK for trunk?

Ok.

Richard.

> Aldy
Aldy Hernandez Dec. 2, 2016, 8:20 a.m. UTC | #2
On 12/01/2016 07:32 AM, Richard Biener wrote:
> On Thu, Dec 1, 2016 at 12:03 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
>> This looks like a latent problem in the -Wmaybe-uninitialized code unrelated
>> to my work.
>>
>> The problem here is a sequence of simplify_preds() followed by
>> normalize_preds() that I added, but is based on prior art all over the file:
>>
>> +      simplify_preds (&uninit_preds, NULL, false);
>> +      uninit_preds = normalize_preds (uninit_preds, NULL, false);
>>
>> The problem is that in this particular testcase we trigger simplify_preds_4
>> which makes a copy of a chain, frees the chain, and then tries to use the
>> chain later (in normalize_preds).  The normalize_preds() function tries to
>> free again the chain and we blow up:
>>
>> This is the main problem in simplify_preds_4:
>>
>>   /* Now clean up the chain.  */
>>   if (simplified)
>>     {
>>       for (i = 0; i < n; i++)
>>         {
>>           if ((*preds)[i].is_empty ())
>>             continue;
>>           s_preds.safe_push ((*preds)[i]);
>> // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> // Makes a copy of the pred_chain.
>>         }
>>
>>       destroy_predicate_vecs (preds);
>> // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> // free() all the pred_chain's.
>>
>>       (*preds) = s_preds;
>> // ^^^^^^^^^^^^^^^^^^^^^^
>> // Wait a minute, we still keep a copy of the pred_chains.
>>       s_preds = vNULL;
>>     }
>>
>> I have no idea how this worked even before my patch.  Perhaps we never had a
>> simplify_preds() followed by a normalize_preds() where the simplification
>> involved a call to simplify_preds_4.
>>
>> Interestingly enough, simplify_preds_2() has the exact same code, but with
>> the fix I am proposing. So this seems like an oversight.  Also, the fact
>> that the simplification in simplify_preds_2 is more common than the
>> simplification performed in simplify_preds_4 would suggest that
>> simplify_preds_4 was uncommon enough and probably wasn't been used in a
>> simplify_preds/normalize_preds combo.
>>
>> Anyways... I've made some other cleanups to the code, but the main gist of
>> the entire patch is:
>>
>> -      destroy_predicate_vecs (preds);
>> +      preds->release ();
>>
>> That is, release preds, but don't free the associated memory with the
>> pred_chain's therein.
>>
>> This patch is on top of my pending patch here:
>>
>>         https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02900.html
>>
>> Tested on x86-64 Linux.
>>
>> OK for trunk?
>
> Ok.

Thank you Richard.

Since this is dependent on the other patch I will wait until said patch 
is approved to commit.

Aldy

Patch
diff mbox

diff --git a/gcc/testsuite/gcc.dg/uninit-pr78548.c b/gcc/testsuite/gcc.dg/uninit-pr78548.c
new file mode 100644
index 0000000..12e06dd
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-pr78548.c
@@ -0,0 +1,24 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wall -w -O2" } */
+
+char a;
+int b;
+unsigned c, d;
+short e;
+int main_f;
+int main (  ) {
+L0:
+    if ( e )     goto L1;
+    b = c & d || a;
+    if ( !c )     printf ( "", ( long long ) main_f );
+    if ( d || !c )     {
+        printf ( "%llu\n", ( long long ) main );
+        goto L2;
+    }
+    unsigned g = b;
+L1:
+    b = g;
+L2:
+    if ( b )     goto L0;
+  return 0;
+}
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index a648995..b4892c7 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -1774,7 +1774,7 @@  simplify_preds_4 (pred_chain_union *preds)
 	  s_preds.safe_push ((*preds)[i]);
 	}
 
-      destroy_predicate_vecs (preds);
+      preds->release ();
       (*preds) = s_preds;
       s_preds = vNULL;
     }
@@ -2211,10 +2211,9 @@  uninit_uses_cannot_happen (gphi *phi, unsigned uninit_opnds,
 
   /* Look for the control dependencies of all the uninitialized
      operands and build guard predicates describing them.  */
-  unsigned i;
-  pred_chain_union uninit_preds[max_phi_args];
-  memset (uninit_preds, 0, sizeof (pred_chain_union) * phi_args);
-  for (i = 0; i < phi_args; ++i)
+  pred_chain_union uninit_preds;
+  bool ret = true;
+  for (unsigned i = 0; i < phi_args; ++i)
     {
       if (!MASK_TEST_BIT (uninit_opnds, i))
 	continue;
@@ -2226,26 +2225,32 @@  uninit_uses_cannot_happen (gphi *phi, unsigned uninit_opnds,
       int num_calls = 0;
 
       /* Build the control dependency chain for uninit operand `i'...  */
+      uninit_preds = vNULL;
       if (!compute_control_dep_chain (find_dom (e->src),
 				      e->src, dep_chains, &num_chains,
 				      &cur_chain, &num_calls))
-	return false;
+	{
+	  ret = false;
+	  break;
+	}
       /* ...and convert it into a set of predicates.  */
       convert_control_dep_chain_into_preds (dep_chains, num_chains,
-					    &uninit_preds[i]);
+					    &uninit_preds);
       for (size_t j = 0; j < num_chains; ++j)
 	dep_chains[j].release ();
-      simplify_preds (&uninit_preds[i], NULL, false);
-      uninit_preds[i]
-	= normalize_preds (uninit_preds[i], NULL, false);
+      simplify_preds (&uninit_preds, NULL, false);
+      uninit_preds = normalize_preds (uninit_preds, NULL, false);
 
       /* Can the guard for this uninitialized operand be invalidated
 	 by the PHI use?  */
-      if (!can_chain_union_be_invalidated_p (uninit_preds[i],
-					     phi_use_guards[0]))
-	return false;
+      if (!can_chain_union_be_invalidated_p (uninit_preds, phi_use_guards[0]))
+	{
+	  ret = false;
+	  break;
+	}
     }
-  return true;
+  destroy_predicate_vecs (&uninit_preds);
+  return ret;
 }
 
 /* Computes the predicates that guard the use and checks