[PATCH] PR c++/68795: fix uninitialized close_paren_loc in cp_parser_postfix_expression

[PATCH] PR c++/68795: fix uninitialized close_paren_loc in cp_parser_postfix_expression

[David Malcolm]

cp_parser_parenthesized_expression_list can leave *close_paren_loc
untouched if an error occurs; specifically when following this goto:

7402              if (expr == error_mark_node)
7403                goto skip_comma;

which can lead to cp_parser_postfix_expression attempting to
use uninitialized data for the finishing location of a
parenthesized expression.

The attached patch fixes this by having cp_parser_postfix_expression
initialize the underlying location to UNKNOWN_LOCATION, and only use
it if it's been written to.

Verified the fix manually by compiling
  g++.old-deja/g++.ns/invalid1.C
before and after under valgrind.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

OK for trunk?

gcc/cp/ChangeLog:
	* parser.c (cp_parser_postfix_expression): Initialize
	close_paren_loc to UNKNOWN_LOCATION; only use it if
	it has been written to by
	cp_parser_parenthesized_expression_list.
	(cp_parser_postfix_dot_deref_expression): Likewise.
	(cp_parser_parenthesized_expression_list): Document the behavior
	with respect to the CLOSE_PAREN_LOC param.
---
 gcc/cp/parser.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index a420cf1..56dfe42 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6664,7 +6664,7 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 	    bool saved_non_integral_constant_expression_p = false;
 	    tsubst_flags_t complain = complain_flags (decltype_p);
 	    vec<tree, va_gc> *args;
-	    location_t close_paren_loc;
+	    location_t close_paren_loc = UNKNOWN_LOCATION;
 
             is_member_access = false;
 
@@ -6826,10 +6826,13 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 				    koenig_p,
 				    complain);
 
-	    location_t combined_loc = make_location (token->location,
-						     start_loc,
-						     close_paren_loc);
-	    postfix_expression.set_location (combined_loc);
+	    if (close_paren_loc)
+	      {
+		location_t combined_loc = make_location (token->location,
+							 start_loc,
+							 close_paren_loc);
+		postfix_expression.set_location (combined_loc);
+	      }
 
 	    /* The POSTFIX_EXPRESSION is certainly no longer an id.  */
 	    idk = CP_ID_KIND_NONE;
@@ -7298,7 +7301,10 @@ cp_parser_postfix_dot_deref_expression (cp_parser *parser,
    plain identifier argument, normal_attr for an attribute that wants
    an expression, or non_attr if we aren't parsing an attribute list.  If
    NON_CONSTANT_P is non-NULL, *NON_CONSTANT_P indicates whether or
-   not all of the expressions in the list were constant.  */
+   not all of the expressions in the list were constant.
+   If CLOSE_PAREN_LOC is non-NULL, and no errors occur, then *CLOSE_PAREN_LOC
+   will be written to with the location of the closing parenthesis.  If
+   an error occurs, it may or may not be written to.  */
 
 static vec<tree, va_gc> *
 cp_parser_parenthesized_expression_list (cp_parser* parser,
-- 
1.8.5.3

Re: [PATCH] PR c++/68795: fix uninitialized close_paren_loc in cp_parser_postfix_expression

[Bernd Schmidt]

On 12/17/2015 07:32 PM, David Malcolm wrote:
> +	    if (close_paren_loc)

close_paren_loc != UNKNOWN_LOCATION - it's very confusing otherwise.


Bernd

[PATCH] PR c++/68795: fix uninitialized close_paren_loc in cp_parser_postfix_expression (v2)

[David Malcolm]

On Thu, 2015-12-17 at 19:21 +0100, Bernd Schmidt wrote:
> On 12/17/2015 07:32 PM, David Malcolm wrote:
> > +             if (close_paren_loc)
> 
> close_paren_loc != UNKNOWN_LOCATION - it's very confusing otherwise.
> 
> 
> Bernd

Here's an updated version; the only change since v1 is:
-           if (close_paren_loc)
+           if (close_paren_loc != UNKNOWN_LOCATION)

Have verified the fix in valgrind.  OK for trunk if it still passes
bootstrap&regrtest?

gcc/cp/ChangeLog:
	* parser.c (cp_parser_postfix_expression): Initialize
	close_paren_loc to UNKNOWN_LOCATION; only use it if
	it has been written to by
	cp_parser_parenthesized_expression_list.
	(cp_parser_postfix_dot_deref_expression): Likewise.
	(cp_parser_parenthesized_expression_list): Document the behavior
	with respect to the CLOSE_PAREN_LOC param.
---
 gcc/cp/parser.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index a420cf1..ae61d8a 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6664,7 +6664,7 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 	    bool saved_non_integral_constant_expression_p = false;
 	    tsubst_flags_t complain = complain_flags (decltype_p);
 	    vec<tree, va_gc> *args;
-	    location_t close_paren_loc;
+	    location_t close_paren_loc = UNKNOWN_LOCATION;
 
             is_member_access = false;
 
@@ -6826,10 +6826,13 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 				    koenig_p,
 				    complain);
 
-	    location_t combined_loc = make_location (token->location,
-						     start_loc,
-						     close_paren_loc);
-	    postfix_expression.set_location (combined_loc);
+	    if (close_paren_loc != UNKNOWN_LOCATION)
+	      {
+		location_t combined_loc = make_location (token->location,
+							 start_loc,
+							 close_paren_loc);
+		postfix_expression.set_location (combined_loc);
+	      }
 
 	    /* The POSTFIX_EXPRESSION is certainly no longer an id.  */
 	    idk = CP_ID_KIND_NONE;
@@ -7298,7 +7301,10 @@ cp_parser_postfix_dot_deref_expression (cp_parser *parser,
    plain identifier argument, normal_attr for an attribute that wants
    an expression, or non_attr if we aren't parsing an attribute list.  If
    NON_CONSTANT_P is non-NULL, *NON_CONSTANT_P indicates whether or
-   not all of the expressions in the list were constant.  */
+   not all of the expressions in the list were constant.
+   If CLOSE_PAREN_LOC is non-NULL, and no errors occur, then *CLOSE_PAREN_LOC
+   will be written to with the location of the closing parenthesis.  If
+   an error occurs, it may or may not be written to.  */
 
 static vec<tree, va_gc> *
 cp_parser_parenthesized_expression_list (cp_parser* parser,
-- 
1.8.5.3

Re: [PATCH] PR c++/68795: fix uninitialized close_paren_loc in cp_parser_postfix_expression (v2)

[Bernd Schmidt]

On 12/18/2015 08:05 PM, David Malcolm wrote:
> On Thu, 2015-12-17 at 19:21 +0100, Bernd Schmidt wrote:
>> On 12/17/2015 07:32 PM, David Malcolm wrote:
>>> +             if (close_paren_loc)
>>
>> close_paren_loc != UNKNOWN_LOCATION - it's very confusing otherwise.
>>
>>
>> Bernd
>
> Here's an updated version; the only change since v1 is:
> -           if (close_paren_loc)
> +           if (close_paren_loc != UNKNOWN_LOCATION)
>
> Have verified the fix in valgrind.  OK for trunk if it still passes
> bootstrap&regrtest?
>
> gcc/cp/ChangeLog:
> 	* parser.c (cp_parser_postfix_expression): Initialize
> 	close_paren_loc to UNKNOWN_LOCATION; only use it if
> 	it has been written to by
> 	cp_parser_parenthesized_expression_list.
> 	(cp_parser_postfix_dot_deref_expression): Likewise.
> 	(cp_parser_parenthesized_expression_list): Document the behavior
> 	with respect to the CLOSE_PAREN_LOC param.

I usually like to leave C++ patches to Jason, but I guess I don't need 
to know the standard inside out for this one.

Prior to your ealier patch, we had

protected_set_expr_location (postfix_expression, token->location);

It looks like after your new patch, we could end up not setting the 
location on the expr at all if close_paren_loc is still 
UNKNOWN_LOCATION. I'm guessing this is intentional as the comment update 
suggests this is an error path, and the cp_expr constructor ensures that 
we get at least UNKNOWN_LOCATION and not another uninitialized loc. If 
I'm correct in all that, the patch is ok.


Bernd

Re: [PATCH] PR c++/68795: fix uninitialized close_paren_loc in cp_parser_postfix_expression (v2)

[David Malcolm]

On Mon, 2016-01-11 at 16:51 +0100, Bernd Schmidt wrote:
> On 12/18/2015 08:05 PM, David Malcolm wrote:
> > On Thu, 2015-12-17 at 19:21 +0100, Bernd Schmidt wrote:
> >> On 12/17/2015 07:32 PM, David Malcolm wrote:
> >>> +             if (close_paren_loc)
> >>
> >> close_paren_loc != UNKNOWN_LOCATION - it's very confusing otherwise.
> >>
> >>
> >> Bernd
> >
> > Here's an updated version; the only change since v1 is:
> > -           if (close_paren_loc)
> > +           if (close_paren_loc != UNKNOWN_LOCATION)
> >
> > Have verified the fix in valgrind.  OK for trunk if it still passes
> > bootstrap&regrtest?
> >
> > gcc/cp/ChangeLog:
> > 	* parser.c (cp_parser_postfix_expression): Initialize
> > 	close_paren_loc to UNKNOWN_LOCATION; only use it if
> > 	it has been written to by
> > 	cp_parser_parenthesized_expression_list.
> > 	(cp_parser_postfix_dot_deref_expression): Likewise.
> > 	(cp_parser_parenthesized_expression_list): Document the behavior
> > 	with respect to the CLOSE_PAREN_LOC param.
> 
> I usually like to leave C++ patches to Jason, but I guess I don't need 
> to know the standard inside out for this one.
> 
> Prior to your ealier patch, we had
> 
> protected_set_expr_location (postfix_expression, token->location);
> 
> It looks like after your new patch, we could end up not setting the 
> location on the expr at all if close_paren_loc is still 
> UNKNOWN_LOCATION. I'm guessing this is intentional as the comment update 
> suggests this is an error path, and the cp_expr constructor ensures that 
> we get at least UNKNOWN_LOCATION and not another uninitialized loc.

Yes.

> If I'm correct in all that, the patch is ok.

Thanks.  Committed to trunk as r232238, having verified
bootstrap&regrtest, and re-verified the PR under valgrind.

Dave