Re: [PATCH] Fix target_clones attribute handling (PR middle-end/78419)

[Richard Biener]

On November 18, 2016 10:41:06 PM GMT+01:00, Jakub Jelinek  
wrote:
>Hi!
>
>The following testcase ICEs because of buffer overflow in the
>expand_target_clones function.  The main bug is that get_attr_str
>doesn't count the commas in the strings, so while it handles
>__attribute__((target_clones ("avx", "foo", "avx2", "avx512f",
>"default")))
>it doesn't handle
>__attribute__((target_clones ("avx,foo,avx2,avx512f,default")))
>which is also meant to be valid.
>This is fixed by the get_attr_str hunk.
>The rest of the changes are to avoid leaking memory, avoid double
>diagnostics (if targetm.target_option.valid_attribute_p fails,
>it has reported errors already, so there is no point to report
>further error or warning), fixing location_t of the diagnostics
>(targetm.target_option.valid_attribute_p uses input_location),
>and various formatting fixes and cleanups.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Richard.

>2016-11-18  Jakub Jelinek  
>
>   PR middle-end/78419
>   * multiple_target.c (get_attr_len): Start with argnum and increment
>   argnum on every arg.  Use strchr in a loop instead of counting commas
>   manually.
>   (get_attr_str): Increment argnum for every comma in the string.
>   (separate_attrs): Use for instead of while loop, simplify.
>   (expand_target_clones): Rename defenition argument to definition.
>   Free attrs and attr_str even when diagnosing errors.  Temporarily
>   change input_location around targetm.target_option.valid_attribute_p
>   calls.  Don't emit warning or errors if that function fails.
>
>   * gcc.target/i386/pr78419.c: New test.
>
>--- gcc/multiple_target.c.jj   2016-08-29 12:17:37.0 +0200
>+++ gcc/multiple_target.c  2016-11-18 14:14:11.684489030 +0100
>@@ -101,22 +101,18 @@ get_attr_len (tree arglist)
> {
>   tree arg;
>   int str_len_sum = 0;
>-  int argnum = 1;
>+  int argnum = 0;
> 
>   for (arg = arglist; arg; arg = TREE_CHAIN (arg))
> {
>-  unsigned int i;
>   const char *str = TREE_STRING_POINTER (TREE_VALUE (arg));
>-  int len = strlen (str);
>-
>+  size_t len = strlen (str);
>   str_len_sum += len + 1;
>-  if (arg != arglist)
>+  for (const char *p = strchr (str, ','); p; p = strchr (p + 1,
>','))
>   argnum++;
>-  for (i = 0; i < strlen (str); i++)
>-  if (str[i] == ',')
>-argnum++;
>+  argnum++;
> }
>-  if (argnum == 1)
>+  if (argnum <= 1)
> return -1;
>   return str_len_sum;
> }
>@@ -134,8 +130,9 @@ get_attr_str (tree arglist, char *attr_s
>   for (arg = arglist; arg; arg = TREE_CHAIN (arg))
> {
>   const char *str = TREE_STRING_POINTER (TREE_VALUE (arg));
>-
>   size_t len = strlen (str);
>+  for (const char *p = strchr (str, ','); p; p = strchr (p + 1,
>','))
>+  argnum++;
>   memcpy (attr_str + str_len_sum, str, len);
>   attr_str[str_len_sum + len] = TREE_CHAIN (arg) ? ',' : '\0';
>   str_len_sum += len + 1;
>@@ -152,19 +149,16 @@ separate_attrs (char *attr_str, char **a
> {
>   int i = 0;
>   bool has_default = false;
>-  char *attr = strtok (attr_str, ",");
> 
>-  while (attr != NULL)
>+  for (char *attr = strtok (attr_str, ",");
>+   attr != NULL; attr = strtok (NULL, ","))
> {
>   if (strcmp (attr, "default") == 0)
>   {
> has_default = true;
>-attr = strtok (NULL, ",");
> continue;
>   }
>-  attrs[i] = attr;
>-  attr = strtok (NULL, ",");
>-  i++;
>+  attrs[i++] = attr;
> }
>   if (!has_default)
> return -1;
>@@ -235,7 +229,7 @@ create_target_clone (cgraph_node *node,
>create the appropriate clone for each valid target attribute.  */
> 
> static bool
>-expand_target_clones (struct cgraph_node *node, bool defenition)
>+expand_target_clones (struct cgraph_node *node, bool definition)
> {
>   int i;
>   /* Parsing target attributes separated by comma.  */
>@@ -266,6 +260,8 @@ expand_target_clones (struct cgraph_node
> {
>   error_at (DECL_SOURCE_LOCATION (node->decl),
>   "default target was not set");
>+  XDELETEVEC (attrs);
>+  XDELETEVEC (attr_str);
>   return false;
> }
> 
>@@ -286,22 +282,24 @@ expand_target_clones (struct cgraph_node
> 
>   create_new_asm_name (attr, suffix);
>   /* Create new target clone.  */
>-  cgraph_node *new_node = create_target_clone (node, defenition,
>suffix);
>+  cgraph_node *new_node = create_target_clone (node, definition,
>suffix);
>   XDELETEVEC (suffix);
> 
>   /* Set new attribute for the clone.  */
>   tree attributes = make_attribute ("target", attr,
>   DECL_ATTRIBUTES (new_node->decl));
>   DECL_ATTRIBUTES (new_node->decl) = attributes;
>+  location_t saved_loc = input_location;
>+  input_location = DECL_SOURCE_LOCATION (node->decl);
>if (!targetm.target_option.valid_attribute_p (new_node->decl, NULL,

[PATCH] Fix target_clones attribute handling (PR middle-end/78419)

[Jakub Jelinek]

Hi!

The following testcase ICEs because of buffer overflow in the
expand_target_clones function.  The main bug is that get_attr_str
doesn't count the commas in the strings, so while it handles
__attribute__((target_clones ("avx", "foo", "avx2", "avx512f", "default")))
it doesn't handle
__attribute__((target_clones ("avx,foo,avx2,avx512f,default")))
which is also meant to be valid.
This is fixed by the get_attr_str hunk.
The rest of the changes are to avoid leaking memory, avoid double
diagnostics (if targetm.target_option.valid_attribute_p fails,
it has reported errors already, so there is no point to report
further error or warning), fixing location_t of the diagnostics
(targetm.target_option.valid_attribute_p uses input_location),
and various formatting fixes and cleanups.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-11-18  Jakub Jelinek  

PR middle-end/78419
* multiple_target.c (get_attr_len): Start with argnum and increment
argnum on every arg.  Use strchr in a loop instead of counting commas
manually.
(get_attr_str): Increment argnum for every comma in the string.
(separate_attrs): Use for instead of while loop, simplify.
(expand_target_clones): Rename defenition argument to definition.
Free attrs and attr_str even when diagnosing errors.  Temporarily
change input_location around targetm.target_option.valid_attribute_p
calls.  Don't emit warning or errors if that function fails.

* gcc.target/i386/pr78419.c: New test.

--- gcc/multiple_target.c.jj2016-08-29 12:17:37.0 +0200
+++ gcc/multiple_target.c   2016-11-18 14:14:11.684489030 +0100
@@ -101,22 +101,18 @@ get_attr_len (tree arglist)
 {
   tree arg;
   int str_len_sum = 0;
-  int argnum = 1;
+  int argnum = 0;
 
   for (arg = arglist; arg; arg = TREE_CHAIN (arg))
 {
-  unsigned int i;
   const char *str = TREE_STRING_POINTER (TREE_VALUE (arg));
-  int len = strlen (str);
-
+  size_t len = strlen (str);
   str_len_sum += len + 1;
-  if (arg != arglist)
+  for (const char *p = strchr (str, ','); p; p = strchr (p + 1, ','))
argnum++;
-  for (i = 0; i < strlen (str); i++)
-   if (str[i] == ',')
- argnum++;
+  argnum++;
 }
-  if (argnum == 1)
+  if (argnum <= 1)
 return -1;
   return str_len_sum;
 }
@@ -134,8 +130,9 @@ get_attr_str (tree arglist, char *attr_s
   for (arg = arglist; arg; arg = TREE_CHAIN (arg))
 {
   const char *str = TREE_STRING_POINTER (TREE_VALUE (arg));
-
   size_t len = strlen (str);
+  for (const char *p = strchr (str, ','); p; p = strchr (p + 1, ','))
+   argnum++;
   memcpy (attr_str + str_len_sum, str, len);
   attr_str[str_len_sum + len] = TREE_CHAIN (arg) ? ',' : '\0';
   str_len_sum += len + 1;
@@ -152,19 +149,16 @@ separate_attrs (char *attr_str, char **a
 {
   int i = 0;
   bool has_default = false;
-  char *attr = strtok (attr_str, ",");
 
-  while (attr != NULL)
+  for (char *attr = strtok (attr_str, ",");
+   attr != NULL; attr = strtok (NULL, ","))
 {
   if (strcmp (attr, "default") == 0)
{
  has_default = true;
- attr = strtok (NULL, ",");
  continue;
}
-  attrs[i] = attr;
-  attr = strtok (NULL, ",");
-  i++;
+  attrs[i++] = attr;
 }
   if (!has_default)
 return -1;
@@ -235,7 +229,7 @@ create_target_clone (cgraph_node *node,
create the appropriate clone for each valid target attribute.  */
 
 static bool
-expand_target_clones (struct cgraph_node *node, bool defenition)
+expand_target_clones (struct cgraph_node *node, bool definition)
 {
   int i;
   /* Parsing target attributes separated by comma.  */
@@ -266,6 +260,8 @@ expand_target_clones (struct cgraph_node
 {
   error_at (DECL_SOURCE_LOCATION (node->decl),
"default target was not set");
+  XDELETEVEC (attrs);
+  XDELETEVEC (attr_str);
   return false;
 }
 
@@ -286,22 +282,24 @@ expand_target_clones (struct cgraph_node
 
   create_new_asm_name (attr, suffix);
   /* Create new target clone.  */
-  cgraph_node *new_node = create_target_clone (node, defenition, suffix);
+  cgraph_node *new_node = create_target_clone (node, definition, suffix);
   XDELETEVEC (suffix);
 
   /* Set new attribute for the clone.  */
   tree attributes = make_attribute ("target", attr,
DECL_ATTRIBUTES (new_node->decl));
   DECL_ATTRIBUTES (new_node->decl) = attributes;
+  location_t saved_loc = input_location;
+  input_location = DECL_SOURCE_LOCATION (node->decl);
   if (!targetm.target_option.valid_attribute_p (new_node->decl, NULL,
-   TREE_VALUE (attributes), 0))
+   TREE_VALUE (attributes),
+