Bug 84831 - Invalid memory read in parse_output_constraint
Summary: Invalid memory read in parse_output_constraint
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 8.0.1
: P3 normal
Target Milestone: ---
Assignee: Jakub Jelinek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-12 11:48 UTC by H.J. Lu
Modified: 2018-03-13 08:13 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-03-12 00:00:00


Attachments
gcc8-pr84831.patch (1.10 KB, patch)
2018-03-12 12:29 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description H.J. Lu 2018-03-12 11:48:55 UTC
parse_output_constraint has

  /* Loop through the constraint string.  */
  for (p = constraint + 1; *p; p += CONSTRAINT_LEN (*p, p))

#define CONSTRAINT_LEN(c_,s_) insn_constraint_len (c_,s_)

On x86, there are

static inline size_t
insn_constraint_len (char fc, const char *str ATTRIBUTE_UNUSED)
{
  switch (fc)
    {   
    case 'B': return 2;
    case 'T': return 2;
    case 'W': return 2;
    case 'Y': return 2;
    default: break;
    }   
  return 1;
}

For

  asm volatile ("" : "+T,Y" (b));

parse_output_constraint doesn't check if p += CONSTRAINT_LEN (*p, p)
is beyond the end of string.
Comment 1 H.J. Lu 2018-03-12 12:04:20 UTC
I am testing this:

diff --git a/gcc/stmt.c b/gcc/stmt.c
index 457fe7f6f78..3a3ff40b682 100644
--- a/gcc/stmt.c
+++ b/gcc/stmt.c
@@ -222,11 +222,12 @@ parse_output_constraint (const char **constraint_p, int op
erand_num,
      from and written to.  */
   *is_inout = (*p == '+');
 
+  size_t c_len = strlen (constraint);
+
   /* Canonicalize the output constraint so that it begins with `='.  */
   if (p != constraint || *is_inout)
     {
       char *buf;
-      size_t c_len = strlen (constraint);
 
       if (p != constraint)
 	warning (0, "output constraint %qc for operand %d "
@@ -247,7 +248,10 @@ parse_output_constraint (const char **constraint_p, int ope
rand_num,
     }
 
   /* Loop through the constraint string.  */
-  for (p = constraint + 1; *p; p += CONSTRAINT_LEN (*p, p))
+  const char *constraint_end = constraint + c_len;
+  for (p = constraint + 1;
+       p <= constraint_end && *p;
+       p += CONSTRAINT_LEN (*p, p))
     switch (*p)
       {
       case '+':
Comment 2 Jakub Jelinek 2018-03-12 12:17:09 UTC
That is not efficient and is insufficient.  I'll handle this.
Comment 3 Jakub Jelinek 2018-03-12 12:29:52 UTC
Created attachment 43630 [details]
gcc8-pr84831.patch

Untested fix (large only because of the reindentation).
Comment 4 Jakub Jelinek 2018-03-12 12:35:25 UTC
diff -upbd of that for better readability.

--- gcc/stmt.c.jj	2018-01-03 10:19:55.150533956 +0100
+++ gcc/stmt.c	2018-03-12 13:25:03.790733765 +0100
@@ -247,7 +247,8 @@ parse_output_constraint (const char **co
     }
 
   /* Loop through the constraint string.  */
-  for (p = constraint + 1; *p; p += CONSTRAINT_LEN (*p, p))
+  for (p = constraint + 1; *p; )
+    {
     switch (*p)
       {
       case '+':
@@ -304,6 +305,11 @@ parse_output_constraint (const char **co
 	break;
       }
 
+      for (size_t len = CONSTRAINT_LEN (*p, p); len; len--, p++)
+	if (*p == '\0')
+	  break;
+    }
+
   return true;
 }
 

Note, I was wrong about insufficient, your patch was sufficient, I was initially also changing parse_input_constraint but it turned out to be unnecessary.
Comment 5 H.J. Lu 2018-03-12 12:46:17 UTC
(In reply to Jakub Jelinek from comment #4)
> diff -upbd of that for better readability.
> 
> --- gcc/stmt.c.jj	2018-01-03 10:19:55.150533956 +0100
> +++ gcc/stmt.c	2018-03-12 13:25:03.790733765 +0100
> @@ -247,7 +247,8 @@ parse_output_constraint (const char **co
>      }
>  
>    /* Loop through the constraint string.  */
> -  for (p = constraint + 1; *p; p += CONSTRAINT_LEN (*p, p))
> +  for (p = constraint + 1; *p; )
> +    {
>      switch (*p)
>        {
>        case '+':
> @@ -304,6 +305,11 @@ parse_output_constraint (const char **co
>  	break;
>        }
>  
> +      for (size_t len = CONSTRAINT_LEN (*p, p); len; len--, p++)
> +	if (*p == '\0')
> +	  break;
> +    }
> +
>    return true;
>  }
>  
> 

Both patches should work.  I have no preference.  Thanks.
Comment 6 Jakub Jelinek 2018-03-13 08:12:39 UTC
Author: jakub
Date: Tue Mar 13 08:12:07 2018
New Revision: 258478

URL: https://gcc.gnu.org/viewcvs?rev=258478&root=gcc&view=rev
Log:
	PR middle-end/84831
	* stmt.c (parse_output_constraint): If the CONSTRAINT_LEN (*p, p)
	characters starting at p contain '\0' character, don't look beyond
	that.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/stmt.c
Comment 7 Jakub Jelinek 2018-03-13 08:13:23 UTC
Fixed.