[PR,c++/71251] out-of-range parms in tmpl arg substitution

Message ID orwoya293a.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • [PR,c++/71251] out-of-range parms in tmpl arg substitution
Related show

Commit Message

Alexandre Oliva March 17, 2018, 12:13 p.m.
As we go through each of the template parameters, substituting it with
corresponding template arguments, an incorrect argument list might
cause us to index argument vectors past their length (or fail in the
preceding tree checks).  Avoid such dereferences and instead issue an
error (if requested) if we find the argument index to be past the
parameter vector length.

Regstrapped on i686- and x86_64-linux-gnu.  Ok to install?

for  gcc/cp/ChangeLog

	PR c++/71251
	* pt.c (tsubst): Test for and report out-of-range template
	parms.

for  gcc/testsuite/ChangeLog

	PR c++/71251
	* g++.dg/cpp0x/pr71251.C: New.
---
 gcc/cp/pt.c                          |   27 +++++++++++++++++++++++----
 gcc/testsuite/g++.dg/cpp0x/pr71251.C |    9 +++++++++
 2 files changed, 32 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C



-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Comments

Jason Merrill March 20, 2018, 4:58 p.m. | #1
On Sat, Mar 17, 2018 at 8:13 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> As we go through each of the template parameters, substituting it with

> corresponding template arguments, an incorrect argument list might

> cause us to index argument vectors past their length (or fail in the

> preceding tree checks).  Avoid such dereferences and instead issue an

> error (if requested) if we find the argument index to be past the

> parameter vector length.


Any time we hit this abort, it indicates a bug in earlier processing,
so that we're looking up a template parameter in an argument list for
a different template.  Aborting in that situation is appropriate; it
has revealed many bugs.

Jason
Alexandre Oliva March 20, 2018, 8:15 p.m. | #2
On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Sat, Mar 17, 2018 at 8:13 AM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> As we go through each of the template parameters, substituting it with

>> corresponding template arguments, an incorrect argument list might

>> cause us to index argument vectors past their length (or fail in the

>> preceding tree checks).  Avoid such dereferences and instead issue an

>> error (if requested) if we find the argument index to be past the

>> parameter vector length.


> Any time we hit this abort, it indicates a bug in earlier processing,

> so that we're looking up a template parameter in an argument list for

> a different template.


That doesn't seem to be the case here.  The argument list given for U is
<T>, as in the testcase, the problem is that U is misdeclared as taking
<int,int> <typename> (two template levels for a single template).

Should we aim at rejecting the declaration of U?

template<int,int> template<typename> using U=void;
template<typename,typename> struct S1;
template<typename T> struct S1<T,U<T>>{ template<typename> struct S2:S2<T>{}; };

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 20, 2018, 8:38 p.m. | #3
On Tue, Mar 20, 2018 at 4:15 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> On Sat, Mar 17, 2018 at 8:13 AM, Alexandre Oliva <aoliva@redhat.com> wrote:

>>> As we go through each of the template parameters, substituting it with

>>> corresponding template arguments, an incorrect argument list might

>>> cause us to index argument vectors past their length (or fail in the

>>> preceding tree checks).  Avoid such dereferences and instead issue an

>>> error (if requested) if we find the argument index to be past the

>>> parameter vector length.

>

>> Any time we hit this abort, it indicates a bug in earlier processing,

>> so that we're looking up a template parameter in an argument list for

>> a different template.

>

> That doesn't seem to be the case here.  The argument list given for U is

> <T>, as in the testcase, the problem is that U is misdeclared as taking

> <int,int> <typename> (two template levels for a single template).

>

> Should we aim at rejecting the declaration of U?


Yes.

Jason
Alexandre Oliva March 20, 2018, 10:51 p.m. | #4
On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Tue, Mar 20, 2018 at 4:15 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> Should we aim at rejecting the declaration of U?


> Yes.


Like this?

[PR c++/71251] check tmpl parms in template using decl

Check that template using decls have the correct number of parm lists.

Will regstrap shortly, ok to install if it passes?

for  gcc/cp/ChangeLog

	PR c++/71251
	* parser.c (cp_parser_alias_declaration): Call
	parser_check_template_parameters.

for  gcc/testsuite/ChangeLog

	PR c++/71251
	* g++.dg/cpp0x/pr71251.C: New.
---
 gcc/cp/parser.c                      |   10 ++++++++++
 gcc/testsuite/g++.dg/cpp0x/pr71251.C |   15 +++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index ce05615adfba..8fa6a37c82f0 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -18910,6 +18910,13 @@ cp_parser_alias_declaration (cp_parser* parser)
   if (id == error_mark_node)
     return error_mark_node;
 
+  if (parser->num_template_parameter_lists
+      && !cp_parser_check_template_parameters (parser,
+					       /*num_templates=*/0,
+					       id_location,
+					       /*declarator=*/NULL))
+    id = error_mark_node;
+
   cp_token *attrs_token = cp_lexer_peek_token (parser->lexer);
   attributes = cp_parser_attributes_opt (parser);
   if (attributes == error_mark_node)
@@ -18980,6 +18987,9 @@ cp_parser_alias_declaration (cp_parser* parser)
 			       ds_alias,
 			       using_token);
 
+  if (id == error_mark_node)
+    return error_mark_node;
+
   declarator = make_id_declarator (NULL_TREE, id, sfk_none);
   declarator->id_loc = id_location;
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/pr71251.C b/gcc/testsuite/g++.dg/cpp0x/pr71251.C
new file mode 100644
index 000000000000..3df831bb581d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/pr71251.C
@@ -0,0 +1,15 @@
+// { dg-do compile { target c++11 } }
+
+template<int,int> template<typename>
+using U = void; // { dg-error "too many" }
+
+template<typename>
+using V = void;
+
+template<typename> struct X {
+  template<typename> template<typename>
+  using U = void; // { dg-error "too many" }
+
+  template<typename>
+  using V = void;
+};


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 21, 2018, 1:44 a.m. | #5
On Tue, Mar 20, 2018 at 6:51 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> On Tue, Mar 20, 2018 at 4:15 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>>> Should we aim at rejecting the declaration of U?

>

>> Yes.

>

> Like this?

>

> [PR c++/71251] check tmpl parms in template using decl

>

> Check that template using decls have the correct number of parm lists.

>

> Will regstrap shortly, ok to install if it passes?

>

> for  gcc/cp/ChangeLog

>

>         PR c++/71251

>         * parser.c (cp_parser_alias_declaration): Call

>         parser_check_template_parameters.

>

> for  gcc/testsuite/ChangeLog

>

>         PR c++/71251

>         * g++.dg/cpp0x/pr71251.C: New.

> ---

>  gcc/cp/parser.c                      |   10 ++++++++++

>  gcc/testsuite/g++.dg/cpp0x/pr71251.C |   15 +++++++++++++++

>  2 files changed, 25 insertions(+)

>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C

>

> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c

> index ce05615adfba..8fa6a37c82f0 100644

> --- a/gcc/cp/parser.c

> +++ b/gcc/cp/parser.c

> @@ -18910,6 +18910,13 @@ cp_parser_alias_declaration (cp_parser* parser)

>    if (id == error_mark_node)

>      return error_mark_node;

>

> +  if (parser->num_template_parameter_lists

> +      && !cp_parser_check_template_parameters (parser,

> +                                              /*num_templates=*/0,

> +                                              id_location,

> +                                              /*declarator=*/NULL))

> +    id = error_mark_node;

>    cp_token *attrs_token = cp_lexer_peek_token (parser->lexer);

>    attributes = cp_parser_attributes_opt (parser);

>    if (attributes == error_mark_node)

> @@ -18980,6 +18987,9 @@ cp_parser_alias_declaration (cp_parser* parser)

>                                ds_alias,

>                                using_token);

>

> +  if (id == error_mark_node)

> +    return error_mark_node;


Why wait until here to return?  There are error returns immediately
above and below your first hunk.

Jason
Alexandre Oliva March 21, 2018, 2:29 a.m. | #6
On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>> +  if (id == error_mark_node)

>> +    return error_mark_node;


> Why wait until here to return?  There are error returns immediately

> above and below your first hunk.


QOI.  Returning immediately, we then get other errors.  We could consume
tokens till the end of the declaration, but I figured we might as well
try to parse them and see whether there were any other legitimate errors
to report.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 22, 2018, 6 p.m. | #7
On Tue, Mar 20, 2018 at 10:29 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>>> +  if (id == error_mark_node)

>>> +    return error_mark_node;

>

>> Why wait until here to return?  There are error returns immediately

>> above and below your first hunk.

>

> QOI.  Returning immediately, we then get other errors.  We could consume

> tokens till the end of the declaration, but I figured we might as well

> try to parse them and see whether there were any other legitimate errors

> to report.


It just seems a bit odd to have the check and the return so far apart.
Do they need to be separate at all?
I think we definitely want to move the check down below the
cp_parser_commit_to_tentative_parse.

Jason
Alexandre Oliva March 22, 2018, 11:12 p.m. | #8
On Mar 22, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Tue, Mar 20, 2018 at 10:29 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>> 

>>>> +  if (id == error_mark_node)

>>>> +    return error_mark_node;

>> 

>>> Why wait until here to return?  There are error returns immediately

>>> above and below your first hunk.

>> 

>> QOI.  Returning immediately, we then get other errors.  We could consume

>> tokens till the end of the declaration, but I figured we might as well

>> try to parse them and see whether there were any other legitimate errors

>> to report.


> It just seems a bit odd to have the check and the return so far apart.

> Do they need to be separate at all?


That depends on how much cp_parser_check_template_parameters depends on
the parser state; I worried moving it down might cause the template
state to have changed enough that the test wouldn't be testing what we
wanted any more, so I kept it right after parsing the identifier.

> I think we definitely want to move the check down below the

> cp_parser_commit_to_tentative_parse.


If you say that won't get it the wrong context for the test, sure.  I'll
run a test cycle with that change.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Alexandre Oliva March 23, 2018, 4:04 a.m. | #9
On Mar 22, 2018, Jason Merrill <jason@redhat.com> wrote:

> I think we definitely want to move the check down below the

> cp_parser_commit_to_tentative_parse.


This worked.  Regstrapped on i686- and x86_64-linux-gnu.  Ok to install?

[PR c++/71251] check tmpl parms in template using decl

Check that template using decls have the correct number of parm lists.

for  gcc/cp/ChangeLog

	PR c++/71251
	* parser.c (cp_parser_alias_declaration): Call
	parser_check_template_parameters.

for  gcc/testsuite/ChangeLog

	PR c++/71251
	* g++.dg/cpp0x/pr71251.C: New.
---
 gcc/cp/parser.c                      |    7 +++++++
 gcc/testsuite/g++.dg/cpp0x/pr71251.C |   15 +++++++++++++++
 2 files changed, 22 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index fd817024eacf..602cc991ff6e 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -18965,6 +18965,13 @@ cp_parser_alias_declaration (cp_parser* parser)
 			       ds_alias,
 			       using_token);
 
+  if (parser->num_template_parameter_lists
+      && !cp_parser_check_template_parameters (parser,
+					       /*num_templates=*/0,
+					       id_location,
+					       /*declarator=*/NULL))
+    return error_mark_node;
+
   declarator = make_id_declarator (NULL_TREE, id, sfk_none);
   declarator->id_loc = id_location;
 
diff --git a/gcc/testsuite/g++.dg/cpp0x/pr71251.C b/gcc/testsuite/g++.dg/cpp0x/pr71251.C
new file mode 100644
index 000000000000..3df831bb581d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/pr71251.C
@@ -0,0 +1,15 @@
+// { dg-do compile { target c++11 } }
+
+template<int,int> template<typename>
+using U = void; // { dg-error "too many" }
+
+template<typename>
+using V = void;
+
+template<typename> struct X {
+  template<typename> template<typename>
+  using U = void; // { dg-error "too many" }
+
+  template<typename>
+  using V = void;
+};


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 23, 2018, 4:06 a.m. | #10
OK.

On Fri, Mar 23, 2018 at 12:04 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 22, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> I think we definitely want to move the check down below the

>> cp_parser_commit_to_tentative_parse.

>

> This worked.  Regstrapped on i686- and x86_64-linux-gnu.  Ok to install?

>

> [PR c++/71251] check tmpl parms in template using decl

>

> Check that template using decls have the correct number of parm lists.

>

> for  gcc/cp/ChangeLog

>

>         PR c++/71251

>         * parser.c (cp_parser_alias_declaration): Call

>         parser_check_template_parameters.

>

> for  gcc/testsuite/ChangeLog

>

>         PR c++/71251

>         * g++.dg/cpp0x/pr71251.C: New.

> ---

>  gcc/cp/parser.c                      |    7 +++++++

>  gcc/testsuite/g++.dg/cpp0x/pr71251.C |   15 +++++++++++++++

>  2 files changed, 22 insertions(+)

>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr71251.C

>

> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c

> index fd817024eacf..602cc991ff6e 100644

> --- a/gcc/cp/parser.c

> +++ b/gcc/cp/parser.c

> @@ -18965,6 +18965,13 @@ cp_parser_alias_declaration (cp_parser* parser)

>                                ds_alias,

>                                using_token);

>

> +  if (parser->num_template_parameter_lists

> +      && !cp_parser_check_template_parameters (parser,

> +                                              /*num_templates=*/0,

> +                                              id_location,

> +                                              /*declarator=*/NULL))

> +    return error_mark_node;

> +

>    declarator = make_id_declarator (NULL_TREE, id, sfk_none);

>    declarator->id_loc = id_location;

>

> diff --git a/gcc/testsuite/g++.dg/cpp0x/pr71251.C b/gcc/testsuite/g++.dg/cpp0x/pr71251.C

> new file mode 100644

> index 000000000000..3df831bb581d

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/cpp0x/pr71251.C

> @@ -0,0 +1,15 @@

> +// { dg-do compile { target c++11 } }

> +

> +template<int,int> template<typename>

> +using U = void; // { dg-error "too many" }

> +

> +template<typename>

> +using V = void;

> +

> +template<typename> struct X {

> +  template<typename> template<typename>

> +  using U = void; // { dg-error "too many" }

> +

> +  template<typename>

> +  using V = void;

> +};

>

>

> --

> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/

> You must be the change you wish to see in the world. -- Gandhi

> Be Free! -- http://FSFLA.org/   FSF Latin America board member

> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Patch

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index fa9bfb12c297..4cc18d0abe78 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -13976,11 +13976,30 @@  tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 	if (level <= levels
 	    && TREE_VEC_LENGTH (TMPL_ARGS_LEVEL (args, level)) > 0)
 	  {
-	    arg = TMPL_ARG (args, level, idx);
+	    if (TREE_VEC_LENGTH (TMPL_ARGS_LEVEL (args, level)) > idx)
+	      {
+		arg = TMPL_ARG (args, level, idx);
 
-	    /* See through ARGUMENT_PACK_SELECT arguments. */
-	    if (arg && TREE_CODE (arg) == ARGUMENT_PACK_SELECT)
-	      arg = argument_pack_select_arg (arg);
+		/* See through ARGUMENT_PACK_SELECT arguments. */
+		if (arg && TREE_CODE (arg) == ARGUMENT_PACK_SELECT)
+		  arg = argument_pack_select_arg (arg);
+	      }
+	    else
+	      {
+		if (complain & tf_error)
+		  {
+		    /* ??? We could use a better location for this
+		       message.  It takes the context of the closing
+		       bracket of the innermost template we're
+		       substituting, but the error may very well be
+		       related with some enclosing context.  */
+		    error ("missing template argument at this"
+			   " or enclosing context");
+		    error_at (DECL_SOURCE_LOCATION (TEMPLATE_PARM_DECL (t)),
+			      "for substitution of template parameter");
+		  }
+		arg = error_mark_node;
+	      }
 	  }
 
 	if (arg == error_mark_node)
diff --git a/gcc/testsuite/g++.dg/cpp0x/pr71251.C b/gcc/testsuite/g++.dg/cpp0x/pr71251.C
new file mode 100644
index 000000000000..f16fb13cccc0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/pr71251.C
@@ -0,0 +1,9 @@ 
+// { dg-do compile { target c++11 } }
+
+template<int,int> template<typename> using U=void; // { dg-error "parameter" }
+
+template<typename,typename> struct S1;
+
+template<typename T> struct S1<T,U<
+  T>>{ template<typename> struct S2:S2<T>{}; }; // { dg-error "missing" }
+//^ the error should be here, but it is^here, thus the weird line break