MIPS: Prevent buffer overrun in uninitialised variable fix

Message ID 6D39441BF12EF246A7ABCE6654B0235380C0761B@HHMAIL01.hh.imgtec.org
State New
Headers show

Commit Message

Matthew Fortune April 20, 2017, 12:49 p.m.
Hi Jeff,

I missed a load of test failures while on vacation and just noticed
that the fix you did for a potentially uninitialized variable warning
is overwriting the stack and breaking MSA in MIPS.

I guess you may have intended to set the length appropriately and
only zero the elements that would not be set in the loop:

memset (&orig_perm[nelt], 0, MAX_VECT_LEN - nelt);

but I switched it to just zero initialise the whole array for
simplicity in the patch below.

I thought I'd check with you before committing. Obviously I'd like to
apply this to GCC 7 branch as well.

Thanks,
Matthew

gcc/
	* config/mips/mips.c (mips_expand_vec_perm_const): Re-fix
	uninitialized variable warning to avoid buffer overrun.
---
 gcc/ChangeLog          | 5 +++++
 gcc/config/mips/mips.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

Comments

Jakub Jelinek April 20, 2017, 12:55 p.m. | #1
On Thu, Apr 20, 2017 at 12:49:49PM +0000, Matthew Fortune wrote:
> Hi Jeff,
> 
> I missed a load of test failures while on vacation and just noticed
> that the fix you did for a potentially uninitialized variable warning
> is overwriting the stack and breaking MSA in MIPS.
> 
> I guess you may have intended to set the length appropriately and
> only zero the elements that would not be set in the loop:
> 
> memset (&orig_perm[nelt], 0, MAX_VECT_LEN - nelt);
> 
> but I switched it to just zero initialise the whole array for
> simplicity in the patch below.
> 
> I thought I'd check with you before committing. Obviously I'd like to
> apply this to GCC 7 branch as well.

As it is just 16, it is not a big deal either way, memsetting everything
might be even faster.  If it would be bigger than that, the MAX_VECT_LEN -
nelt case would be better.

> gcc/
> 	* config/mips/mips.c (mips_expand_vec_perm_const): Re-fix
> 	uninitialized variable warning to avoid buffer overrun.

Ok, thanks, even for GCC 7 branch.

	Jakub
Matthew Fortune April 20, 2017, 1:40 p.m. | #2
Jakub Jelinek <jakub@redhat.com> writes:
> On Thu, Apr 20, 2017 at 12:49:49PM +0000, Matthew Fortune wrote:
> > Hi Jeff,
> >
> > I missed a load of test failures while on vacation and just noticed
> > that the fix you did for a potentially uninitialized variable warning
> > is overwriting the stack and breaking MSA in MIPS.
> >
> > I guess you may have intended to set the length appropriately and only
> > zero the elements that would not be set in the loop:
> >
> > memset (&orig_perm[nelt], 0, MAX_VECT_LEN - nelt);
> >
> > but I switched it to just zero initialise the whole array for
> > simplicity in the patch below.
> >
> > I thought I'd check with you before committing. Obviously I'd like to
> > apply this to GCC 7 branch as well.
> 
> As it is just 16, it is not a big deal either way, memsetting everything
> might be even faster.  If it would be bigger than that, the MAX_VECT_LEN
> - nelt case would be better.
> 
> > gcc/
> > 	* config/mips/mips.c (mips_expand_vec_perm_const): Re-fix
> > 	uninitialized variable warning to avoid buffer overrun.
> 
> Ok, thanks, even for GCC 7 branch.

Thanks Jakub. Committed on trunk and gcc-7-branch.

Matthew

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index c908048..80d3436 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@ 
+2017-04-20  Matthew Fortune  <matthew.fortune@imgtec.com>
+
+	* config/mips/mips.c (mips_expand_vec_perm_const): Re-fix
+	uninitialized variable warning to avoid buffer overrun.
+
 2017-04-20  Alexander Monakov  <amonakov@ispras.ru>
 
 	PR other/71250
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index b35fba7..6bfd86a 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -21358,7 +21358,7 @@  mips_expand_vec_perm_const (rtx operands[4])
 
   /* This is overly conservative, but ensures we don't get an
      uninitialized warning on ORIG_PERM.  */
-  memset (&orig_perm[nelt], 0, MAX_VECT_LEN);
+  memset (orig_perm, 0, MAX_VECT_LEN);
   for (i = which = 0; i < nelt; ++i)
     {
       rtx e = XVECEXP (sel, 0, i);