Fix c/69522, memory management issue in c-parser

Fix c/69522, memory management issue in c-parser

[Bernd Schmidt]

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

Let's say we have

struct a {
  int x[1];
  int y[1];
} x = { 0, { 0 } };
            ^

When we reach the marked brace, we call into push_init_level, where we 
notice that we have implicit initializers (for x[]) lying around that we 
should deal with now that we've seen another open brace. The problem is 
that we've created a new obstack for the initializer of y, and this is 
where we also put data for the inits of x, freeing it when we see the 
close brace for the initialization of y.

In the actual testcase, which is a little more complex to actually 
demonstrate the issue, we end up allocating two init elts at the same 
address (because of premature freeing) and place them in the same tree, 
which ends up containing a cycle because of this. Then we hang.

Fixed by this patch, which splits off a new function 
finish_implicit_inits from push_init_level and ensures it's called with 
the outer obstack instead of the new one in the problematic case.

Bootstrapped and tested on x86_64-linux, ok?


Bernd

[-- Attachment #2: braced-init.diff --]
[-- Type: text/x-patch, Size: 7100 bytes --]

c/
	PR c/69522
	* c-parser.c (c_parser_braced_init): New arg outer_obstack.  All
	callers changed.  If nested_p is true, use it to call
	finish_implicit_inits.
	* c-tree.h (finish_implicit_inits): Declare.
	* c-typeck.c (finish_implicit_inits): New function.  Move code
	from ...
	(push_init_level): ... here.
	(set_designator, process_init_element): Call finish_implicit_inits.

testsuite/
	PR c/69522
	gcc.dg/pr69522.c: New test.

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 43c26ae..eac0b1c 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -1284,7 +1284,8 @@ static tree c_parser_simple_asm_expr (c_parser *);
 static tree c_parser_attributes (c_parser *);
 static struct c_type_name *c_parser_type_name (c_parser *);
 static struct c_expr c_parser_initializer (c_parser *);
-static struct c_expr c_parser_braced_init (c_parser *, tree, bool);
+static struct c_expr c_parser_braced_init (c_parser *, tree, bool,
+					   struct obstack *);
 static void c_parser_initelt (c_parser *, struct obstack *);
 static void c_parser_initval (c_parser *, struct c_expr *,
 			      struct obstack *);
@@ -4289,7 +4290,7 @@ static struct c_expr
 c_parser_initializer (c_parser *parser)
 {
   if (c_parser_next_token_is (parser, CPP_OPEN_BRACE))
-    return c_parser_braced_init (parser, NULL_TREE, false);
+    return c_parser_braced_init (parser, NULL_TREE, false, NULL);
   else
     {
       struct c_expr ret;
@@ -4309,7 +4310,8 @@ c_parser_initializer (c_parser *parser)
    top-level initializer in a declaration.  */
 
 static struct c_expr
-c_parser_braced_init (c_parser *parser, tree type, bool nested_p)
+c_parser_braced_init (c_parser *parser, tree type, bool nested_p,
+		      struct obstack *outer_obstack)
 {
   struct c_expr ret;
   struct obstack braced_init_obstack;
@@ -4318,7 +4320,10 @@ c_parser_braced_init (c_parser *parser, tree type, bool nested_p)
   gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE));
   c_parser_consume_token (parser);
   if (nested_p)
-    push_init_level (brace_loc, 0, &braced_init_obstack);
+    {
+      finish_implicit_inits (brace_loc, outer_obstack);
+      push_init_level (brace_loc, 0, &braced_init_obstack);
+    }
   else
     really_start_incremental_init (type);
   if (c_parser_next_token_is (parser, CPP_CLOSE_BRACE))
@@ -4576,7 +4581,8 @@ c_parser_initval (c_parser *parser, struct c_expr *after,
   location_t loc = c_parser_peek_token (parser)->location;
 
   if (c_parser_next_token_is (parser, CPP_OPEN_BRACE) && !after)
-    init = c_parser_braced_init (parser, NULL_TREE, true);
+    init = c_parser_braced_init (parser, NULL_TREE, true,
+				 braced_init_obstack);
   else
     {
       init = c_parser_expr_no_commas (parser, after);
@@ -8060,7 +8066,7 @@ c_parser_postfix_expression_after_paren_type (c_parser *parser,
       error_at (type_loc, "compound literal has variable size");
       type = error_mark_node;
     }
-  init = c_parser_braced_init (parser, type, false);
+  init = c_parser_braced_init (parser, type, false, NULL);
   finish_init ();
   maybe_warn_string_init (type_loc, type, init);
 
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index 00e72b1..5902fd2 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -625,6 +625,7 @@ extern void maybe_warn_string_init (location_t, tree, struct c_expr);
 extern void start_init (tree, tree, int);
 extern void finish_init (void);
 extern void really_start_incremental_init (tree);
+extern void finish_implicit_inits (location_t, struct obstack *);
 extern void push_init_level (location_t, int, struct obstack *);
 extern struct c_expr pop_init_level (location_t, int, struct obstack *);
 extern void set_init_index (location_t, tree, tree, struct obstack *);
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index a147ac6..e4e2944 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -7447,6 +7447,30 @@ really_start_incremental_init (tree type)
     }
 }
 \f
+/* Called when we see an open brace for a nested initializer.  Finish
+   off any pending levels with implicit braces.  */
+void
+finish_implicit_inits (location_t loc, struct obstack *braced_init_obstack)
+{
+  while (constructor_stack->implicit)
+    {
+      if (RECORD_OR_UNION_TYPE_P (constructor_type)
+	  && constructor_fields == 0)
+	process_init_element (input_location,
+			      pop_init_level (loc, 1, braced_init_obstack),
+			      true, braced_init_obstack);
+      else if (TREE_CODE (constructor_type) == ARRAY_TYPE
+	       && constructor_max_index
+	       && tree_int_cst_lt (constructor_max_index,
+				   constructor_index))
+	process_init_element (input_location,
+			      pop_init_level (loc, 1, braced_init_obstack),
+			      true, braced_init_obstack);
+      else
+	break;
+    }
+}
+
 /* Push down into a subobject, for initialization.
    If this is for an explicit set of braces, IMPLICIT is 0.
    If it is because the next element belongs at a lower level,
@@ -7459,33 +7483,6 @@ push_init_level (location_t loc, int implicit,
   struct constructor_stack *p;
   tree value = NULL_TREE;
 
-  /* If we've exhausted any levels that didn't have braces,
-     pop them now.  If implicit == 1, this will have been done in
-     process_init_element; do not repeat it here because in the case
-     of excess initializers for an empty aggregate this leads to an
-     infinite cycle of popping a level and immediately recreating
-     it.  */
-  if (implicit != 1)
-    {
-      while (constructor_stack->implicit)
-	{
-	  if (RECORD_OR_UNION_TYPE_P (constructor_type)
-	      && constructor_fields == 0)
-	    process_init_element (input_location,
-				  pop_init_level (loc, 1, braced_init_obstack),
-				  true, braced_init_obstack);
-	  else if (TREE_CODE (constructor_type) == ARRAY_TYPE
-		   && constructor_max_index
-		   && tree_int_cst_lt (constructor_max_index,
-				       constructor_index))
-	    process_init_element (input_location,
-				  pop_init_level (loc, 1, braced_init_obstack),
-				  true, braced_init_obstack);
-	  else
-	    break;
-	}
-    }
-
   /* Unless this is an explicit brace, we need to preserve previous
      content if any.  */
   if (implicit)
@@ -7912,6 +7909,7 @@ set_designator (location_t loc, int array,
     }
 
   constructor_designated = 1;
+  finish_implicit_inits (loc, braced_init_obstack);
   push_init_level (loc, 2, braced_init_obstack);
   return 0;
 }
@@ -9295,6 +9293,7 @@ process_init_element (location_t loc, struct c_expr value, bool implicit,
 	      p = p->next;
 	      if (!p)
 		break;
+	      finish_implicit_inits (loc, braced_init_obstack);
 	      push_init_level (loc, 2, braced_init_obstack);
 	      p->stack = constructor_stack;
 	      if (p->range_end && tree_int_cst_equal (p->index, p->range_end))
Index: gcc/testsuite/gcc.dg/pr69522.c
===================================================================
--- gcc/testsuite/gcc.dg/pr69522.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr69522.c	(working copy)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+struct str {};
+struct {
+  struct str b;
+  float c[1];
+  int d[1];
+  float e[2];
+  int f[1];
+} a = {{}, 0, {0.5}, 0, 0, {0}};

Re: Fix c/69522, memory management issue in c-parser

[Bernd Schmidt]

Ping.

On 01/29/2016 12:40 PM, Bernd Schmidt wrote:
> Let's say we have
>
> struct a {
>   int x[1];
>   int y[1];
> } x = { 0, { 0 } };
>             ^
>
> When we reach the marked brace, we call into push_init_level, where we
> notice that we have implicit initializers (for x[]) lying around that we
> should deal with now that we've seen another open brace. The problem is
> that we've created a new obstack for the initializer of y, and this is
> where we also put data for the inits of x, freeing it when we see the
> close brace for the initialization of y.
>
> In the actual testcase, which is a little more complex to actually
> demonstrate the issue, we end up allocating two init elts at the same
> address (because of premature freeing) and place them in the same tree,
> which ends up containing a cycle because of this. Then we hang.
>
> Fixed by this patch, which splits off a new function
> finish_implicit_inits from push_init_level and ensures it's called with
> the outer obstack instead of the new one in the problematic case.
>
> Bootstrapped and tested on x86_64-linux, ok?
>
>
> Bernd

Re: Fix c/69522, memory management issue in c-parser

[Jeff Law]

On 01/29/2016 04:40 AM, Bernd Schmidt wrote:
> Let's say we have
>
> struct a {
>   int x[1];
>   int y[1];
> } x = { 0, { 0 } };
>             ^
>
> When we reach the marked brace, we call into push_init_level, where we
> notice that we have implicit initializers (for x[]) lying around that we
> should deal with now that we've seen another open brace. The problem is
> that we've created a new obstack for the initializer of y, and this is
> where we also put data for the inits of x, freeing it when we see the
> close brace for the initialization of y.
>
> In the actual testcase, which is a little more complex to actually
> demonstrate the issue, we end up allocating two init elts at the same
> address (because of premature freeing) and place them in the same tree,
> which ends up containing a cycle because of this. Then we hang.
>
> Fixed by this patch, which splits off a new function
> finish_implicit_inits from push_init_level and ensures it's called with
> the outer obstack instead of the new one in the problematic case.
>
> Bootstrapped and tested on x86_64-linux, ok?
>
>
> Bernd
>
> braced-init.diff
>
>
> c/
> 	PR c/69522
> 	* c-parser.c (c_parser_braced_init): New arg outer_obstack.  All
> 	callers changed.  If nested_p is true, use it to call
> 	finish_implicit_inits.
> 	* c-tree.h (finish_implicit_inits): Declare.
> 	* c-typeck.c (finish_implicit_inits): New function.  Move code
> 	from ...
> 	(push_init_level): ... here.
> 	(set_designator, process_init_element): Call finish_implicit_inits.
>
> testsuite/
> 	PR c/69522
> 	gcc.dg/pr69522.c: New test.
OK.  Thanks for tracking this down.

Thanks,
Jeff

Re: Fix c/69522, memory management issue in c-parser

[Andreas Schwab]

FAIL: gcc.dg/pr69522.c (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20160212/gcc/testsuite/gcc.dg/pr69522.c:2:8: error: struct has no members [-Wpedantic]
/daten/aranym/gcc/gcc-20160212/gcc/testsuite/gcc.dg/pr69522.c:9:8: error: ISO C forbids empty initializer braces [-Wpedantic]

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Fix c/69522, memory management issue in c-parser

[Marek Polacek]

On Fri, Feb 12, 2016 at 02:17:19PM +0100, Andreas Schwab wrote:
> FAIL: gcc.dg/pr69522.c (test for excess errors)
> Excess errors:
> /daten/aranym/gcc/gcc-20160212/gcc/testsuite/gcc.dg/pr69522.c:2:8: error: struct has no members [-Wpedantic]
> /daten/aranym/gcc/gcc-20160212/gcc/testsuite/gcc.dg/pr69522.c:9:8: error: ISO C forbids empty initializer braces [-Wpedantic]

Bernd, ok to fix this with the following?

2016-02-12  Marek Polacek  <polacek@redhat.com>

	* gcc.dg/pr69522.c: Add empty dg-options.

diff --git gcc/testsuite/gcc.dg/pr69522.c gcc/testsuite/gcc.dg/pr69522.c
index 452a1ae..820168d 100644
--- gcc/testsuite/gcc.dg/pr69522.c
+++ gcc/testsuite/gcc.dg/pr69522.c
@@ -1,4 +1,5 @@
 /* { dg-do compile } */
+/* { dg-options "" } */
 struct str {};
 struct {
   struct str b;

	Marek

Re: Fix c/69522, memory management issue in c-parser

[Bernd Schmidt]

On 02/12/2016 02:26 PM, Marek Polacek wrote:
> On Fri, Feb 12, 2016 at 02:17:19PM +0100, Andreas Schwab wrote:
>> FAIL: gcc.dg/pr69522.c (test for excess errors)
>> Excess errors:
>> /daten/aranym/gcc/gcc-20160212/gcc/testsuite/gcc.dg/pr69522.c:2:8: error: struct has no members [-Wpedantic]
>> /daten/aranym/gcc/gcc-20160212/gcc/testsuite/gcc.dg/pr69522.c:9:8: error: ISO C forbids empty initializer braces [-Wpedantic]
>
> Bernd, ok to fix this with the following?
>
> 2016-02-12  Marek Polacek  <polacek@redhat.com>
>
> 	* gcc.dg/pr69522.c: Add empty dg-options.

What's causing the problem there, -Wpedantic? Maybe add the opposite 
rather than an empty dg-options? Either way's fine, sorry about the 
breakage - I thought I had the testcase applied during the test run, but 
apparently not.


Bernd

Re: Fix c/69522, memory management issue in c-parser

[Marek Polacek]

On Fri, Feb 12, 2016 at 02:28:39PM +0100, Bernd Schmidt wrote:
> On 02/12/2016 02:26 PM, Marek Polacek wrote:
> >On Fri, Feb 12, 2016 at 02:17:19PM +0100, Andreas Schwab wrote:
> >>FAIL: gcc.dg/pr69522.c (test for excess errors)
> >>Excess errors:
> >>/daten/aranym/gcc/gcc-20160212/gcc/testsuite/gcc.dg/pr69522.c:2:8: error: struct has no members [-Wpedantic]
> >>/daten/aranym/gcc/gcc-20160212/gcc/testsuite/gcc.dg/pr69522.c:9:8: error: ISO C forbids empty initializer braces [-Wpedantic]
> >
> >Bernd, ok to fix this with the following?
> >
> >2016-02-12  Marek Polacek  <polacek@redhat.com>
> >
> >	* gcc.dg/pr69522.c: Add empty dg-options.
> 
> What's causing the problem there, -Wpedantic? Maybe add the opposite rather
> than an empty dg-options? Either way's fine, sorry about the breakage - I
> thought I had the testcase applied during the test run, but apparently not.

Without dg-options we test with -ansi (aka -std=c89) -pedantic-errors, hence
the error.  With empty dg-options we test without these two options.  Empty
dg-options are quite common in the testsuite.  I'll go ahead with the patch.

	Marek

Re: Fix c/69522, memory management issue in c-parser

[Bernd Schmidt]

On 02/08/2016 05:30 PM, Jeff Law wrote:
> On 01/29/2016 04:40 AM, Bernd Schmidt wrote:
>> c/
>>     PR c/69522
>>     * c-parser.c (c_parser_braced_init): New arg outer_obstack.  All
>>     callers changed.  If nested_p is true, use it to call
>>     finish_implicit_inits.
>>     * c-tree.h (finish_implicit_inits): Declare.
>>     * c-typeck.c (finish_implicit_inits): New function.  Move code
>>     from ...
>>     (push_init_level): ... here.
>>     (set_designator, process_init_element): Call finish_implicit_inits.
>>
>> testsuite/
>>     PR c/69522
>>     gcc.dg/pr69522.c: New test.
> OK.  Thanks for tracking this down.

Ok for branches too?


Bernd

Re: Fix c/69522, memory management issue in c-parser

[Jeff Law]

On 02/16/2016 07:21 AM, Bernd Schmidt wrote:
> On 02/08/2016 05:30 PM, Jeff Law wrote:
>> On 01/29/2016 04:40 AM, Bernd Schmidt wrote:
>>> c/
>>>     PR c/69522
>>>     * c-parser.c (c_parser_braced_init): New arg outer_obstack.  All
>>>     callers changed.  If nested_p is true, use it to call
>>>     finish_implicit_inits.
>>>     * c-tree.h (finish_implicit_inits): Declare.
>>>     * c-typeck.c (finish_implicit_inits): New function.  Move code
>>>     from ...
>>>     (push_init_level): ... here.
>>>     (set_designator, process_init_element): Call finish_implicit_inits.
>>>
>>> testsuite/
>>>     PR c/69522
>>>     gcc.dg/pr69522.c: New test.
>> OK.  Thanks for tracking this down.
>
> Ok for branches too?
Yes.  Definitely given it's a memory management issue.

jeff

Re: Fix c/69522, memory management issue in c-parser

[Christophe Lyon]

On 16 February 2016 at 16:37, Jeff Law <law@redhat.com> wrote:
> On 02/16/2016 07:21 AM, Bernd Schmidt wrote:
>>
>> On 02/08/2016 05:30 PM, Jeff Law wrote:
>>>
>>> On 01/29/2016 04:40 AM, Bernd Schmidt wrote:
>>>>
>>>> c/
>>>>     PR c/69522
>>>>     * c-parser.c (c_parser_braced_init): New arg outer_obstack.  All
>>>>     callers changed.  If nested_p is true, use it to call
>>>>     finish_implicit_inits.
>>>>     * c-tree.h (finish_implicit_inits): Declare.
>>>>     * c-typeck.c (finish_implicit_inits): New function.  Move code
>>>>     from ...
>>>>     (push_init_level): ... here.
>>>>     (set_designator, process_init_element): Call finish_implicit_inits.
>>>>
>>>> testsuite/
>>>>     PR c/69522
>>>>     gcc.dg/pr69522.c: New test.
>>>
>>> OK.  Thanks for tracking this down.
>>
>>
>> Ok for branches too?
>
Hi,

FWIW, I'm seeing the new test failing on the 4.9 branch:
gcc/testsuite/gcc.dg/pr69522.c:2:8: error: struct has no members [-Wpedantic]
gcc/testsuite/gcc.dg/pr69522.c:9:8: error: ISO C forbids empty
initializer braces [-Wpedantic]

> Yes.  Definitely given it's a memory management issue.
>
> jeff

Re: Fix c/69522, memory management issue in c-parser

[Marek Polacek]

On Thu, Feb 18, 2016 at 11:53:04AM +0100, Christophe Lyon wrote:
> FWIW, I'm seeing the new test failing on the 4.9 branch:
> gcc/testsuite/gcc.dg/pr69522.c:2:8: error: struct has no members [-Wpedantic]
> gcc/testsuite/gcc.dg/pr69522.c:9:8: error: ISO C forbids empty
> initializer braces [-Wpedantic]

4.9 branch is missing https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00867.html
I'll fix.

	Marek

Re: Fix c/69522, memory management issue in c-parser

[Bernd Schmidt]

On 02/18/2016 12:22 PM, Marek Polacek wrote:
> On Thu, Feb 18, 2016 at 11:53:04AM +0100, Christophe Lyon wrote:
>> FWIW, I'm seeing the new test failing on the 4.9 branch:
>> gcc/testsuite/gcc.dg/pr69522.c:2:8: error: struct has no members [-Wpedantic]
>> gcc/testsuite/gcc.dg/pr69522.c:9:8: error: ISO C forbids empty
>> initializer braces [-Wpedantic]
>
> 4.9 branch is missing https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00867.html
> I'll fix.

I suck.


Bernd