sra: Avoid SRAing if there is an aout-of-bounds access (PR 96820)

Message ID ri6v9h2k8x6.fsf@suse.cz
State New
Headers show
Series
  • sra: Avoid SRAing if there is an aout-of-bounds access (PR 96820)
Related show

Commit Message

Martin Jambor Aug. 28, 2020, 4:54 p.m.
Hi,

the testcase causes and ICE in the SRA verifier on x86_64 when
compiling with -m32 because build_user_friendly_ref_for_offset looks
at an out-of-bounds array_ref within an array_ref which accesses an
offset which does not fit into a signed 32bit integer and turns it
into an array-ref with a negative index.

The best thing is probably to bail out early when encountering an out
of bounds access to a local stack-allocated aggregate (and let the DSE
just delete such statements) which is what the patch does.

I also glanced over to the initial candidate vetting routine to make
sure the size would fit into HWI and noticed that it uses unsigned
variants whereas the rest of SRA operates on signed offsets and
sizes (because get_ref_and_extent does) and so changed that for the
sake of consistency.  These ancient checks operate on sizes of types
as opposed to DECLs but I hope that any issues potentially arising
from that are basically hypothetical.

Bootstrapped and tested on x86_64-linux.  OK for master and then for
gcc-10 branch?

Thanks,

Martin


gcc/ChangeLog:

2020-08-28  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/96820
	* tree-sra.c (create_access): Disqualify candidates with accesses
	beyond the end of the original aggregate.
	(maybe_add_sra_candidate): Check that candidate type size fits
	signed uhwi for the sake of consistency.

gcc/testsuite/ChangeLog:

2020-08-28  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/96820
	* gcc.dg/tree-ssa/pr96820.c: New test.
---
 gcc/testsuite/gcc.dg/tree-ssa/pr96820.c | 12 ++++++++++++
 gcc/tree-sra.c                          |  9 +++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr96820.c

-- 
2.28.0

Comments

Richard Biener Aug. 31, 2020, 7:15 a.m. | #1
On Fri, 28 Aug 2020, Martin Jambor wrote:

> Hi,

> 

> the testcase causes and ICE in the SRA verifier on x86_64 when

> compiling with -m32 because build_user_friendly_ref_for_offset looks

> at an out-of-bounds array_ref within an array_ref which accesses an

> offset which does not fit into a signed 32bit integer and turns it

> into an array-ref with a negative index.

> 

> The best thing is probably to bail out early when encountering an out

> of bounds access to a local stack-allocated aggregate (and let the DSE

> just delete such statements) which is what the patch does.

> 

> I also glanced over to the initial candidate vetting routine to make

> sure the size would fit into HWI and noticed that it uses unsigned

> variants whereas the rest of SRA operates on signed offsets and

> sizes (because get_ref_and_extent does) and so changed that for the

> sake of consistency.  These ancient checks operate on sizes of types

> as opposed to DECLs but I hope that any issues potentially arising

> from that are basically hypothetical.

> 

> Bootstrapped and tested on x86_64-linux.  OK for master and then for

> gcc-10 branch?


OK.

Richard.

> Thanks,

> 

> Martin

> 

> 

> gcc/ChangeLog:

> 

> 2020-08-28  Martin Jambor  <mjambor@suse.cz>

> 

> 	PR tree-optimization/96820

> 	* tree-sra.c (create_access): Disqualify candidates with accesses

> 	beyond the end of the original aggregate.

> 	(maybe_add_sra_candidate): Check that candidate type size fits

> 	signed uhwi for the sake of consistency.

> 

> gcc/testsuite/ChangeLog:

> 

> 2020-08-28  Martin Jambor  <mjambor@suse.cz>

> 

> 	PR tree-optimization/96820

> 	* gcc.dg/tree-ssa/pr96820.c: New test.

> ---

>  gcc/testsuite/gcc.dg/tree-ssa/pr96820.c | 12 ++++++++++++

>  gcc/tree-sra.c                          |  9 +++++++--

>  2 files changed, 19 insertions(+), 2 deletions(-)

>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr96820.c

> 

> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr96820.c b/gcc/testsuite/gcc.dg/tree-ssa/pr96820.c

> new file mode 100644

> index 00000000000..f5c2195f310

> --- /dev/null

> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr96820.c

> @@ -0,0 +1,12 @@

> +/* { dg-do compile } */

> +/* { dg-options "-O1" } */

> +

> +struct a {

> +  int b;

> +};

> +int main() {

> +  struct a d[][6] = {4};

> +  struct a e;

> +  d[1955249013][1955249013] = e;

> +  return e.b;

> +}

> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c

> index 754f41302fc..98a6cacbe2a 100644

> --- a/gcc/tree-sra.c

> +++ b/gcc/tree-sra.c

> @@ -941,6 +941,11 @@ create_access (tree expr, gimple *stmt, bool write)

>        disqualify_candidate (base, "Encountered an unconstrained access.");

>        return NULL;

>      }

> +  if (offset + size > tree_to_shwi (DECL_SIZE (base)))

> +    {

> +      disqualify_candidate (base, "Encountered an access beyond the base.");

> +      return NULL;

> +    }

>  

>    access = create_access_1 (base, offset, size);

>    access->expr = expr;

> @@ -1880,12 +1885,12 @@ maybe_add_sra_candidate (tree var)

>        reject (var, "has incomplete type");

>        return false;

>      }

> -  if (!tree_fits_uhwi_p (TYPE_SIZE (type)))

> +  if (!tree_fits_shwi_p (TYPE_SIZE (type)))

>      {

>        reject (var, "type size not fixed");

>        return false;

>      }

> -  if (tree_to_uhwi (TYPE_SIZE (type)) == 0)

> +  if (tree_to_shwi (TYPE_SIZE (type)) == 0)

>      {

>        reject (var, "type size is zero");

>        return false;

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Patch

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr96820.c b/gcc/testsuite/gcc.dg/tree-ssa/pr96820.c
new file mode 100644
index 00000000000..f5c2195f310
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr96820.c
@@ -0,0 +1,12 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+struct a {
+  int b;
+};
+int main() {
+  struct a d[][6] = {4};
+  struct a e;
+  d[1955249013][1955249013] = e;
+  return e.b;
+}
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 754f41302fc..98a6cacbe2a 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -941,6 +941,11 @@  create_access (tree expr, gimple *stmt, bool write)
       disqualify_candidate (base, "Encountered an unconstrained access.");
       return NULL;
     }
+  if (offset + size > tree_to_shwi (DECL_SIZE (base)))
+    {
+      disqualify_candidate (base, "Encountered an access beyond the base.");
+      return NULL;
+    }
 
   access = create_access_1 (base, offset, size);
   access->expr = expr;
@@ -1880,12 +1885,12 @@  maybe_add_sra_candidate (tree var)
       reject (var, "has incomplete type");
       return false;
     }
-  if (!tree_fits_uhwi_p (TYPE_SIZE (type)))
+  if (!tree_fits_shwi_p (TYPE_SIZE (type)))
     {
       reject (var, "type size not fixed");
       return false;
     }
-  if (tree_to_uhwi (TYPE_SIZE (type)) == 0)
+  if (tree_to_shwi (TYPE_SIZE (type)) == 0)
     {
       reject (var, "type size is zero");
       return false;