Re: [PATH] [BZ 15674] Fix reading past the array boundary in __memcmp_ssse3

["Carlos O'Donell" <carlos at redhat dot com>]

Dmitrieva,

Thanks for fixing this.

Comments below.

On 06/25/2013 10:11 AM, Dmitrieva Liubov wrote:
> ChangeLog:
> 
> 2013-06-25  Liubov Dmitrieva  <liubov.dmitrieva@intel.com>
> 
> [BZ #15674]
> * sysdeps/x86_64/multiarch/memcmp-ssse3.S: Fix
> buffers overrun.
> * string/test-memcmp.c (check2): Add new test.
 
Incorrectly formatted ChangeLog.
 
> 
> diff --git a/string/test-memcmp.c b/string/test-memcmp.c
> index b30e34d..d627888 100644
> --- a/string/test-memcmp.c
> +++ b/string/test-memcmp.c
> @@ -448,6 +448,36 @@ check1 (void)
>      }
>  }
> 

Please add a comment describing what the test does and why it failed.

> +static void
> +check2 (void)
> +{
> +  int max_length = BUF1PAGES * page_size / sizeof (CHAR);
> +
> +  char * buf = (char *) malloc (sizeof (char) * max_length);
> +  // initialize buf to the same values as buf1:

Please use C comments. Capitalize sentence please.

> +  memset (buf, 0xa5, max_length);
> +  // The bug requires the last compared byte to be different.

Likewise.

> +  buf[max_length - 1]  = 0x5a;
> +
> +  int length;
> +
> +  for (length = 1; length < max_length; length++)
> +    {
> +      char * s1 = (char *) buf1 + max_length - length;
> +      char * s2 = buf + max_length - length;
> +
> +      const int exp_result = SIMPLE_MEMCMP (s1, s2, length);
> +
> +      FOR_EACH_IMPL (impl, 0)
> +       {
> +         printf ("check2: length=%d, %s\n", length, impl->name);
> +         check_result (impl, s1, s2, length, exp_result);
> +       }
> +    }
> +
> +  free(buf);
> +}
> +
>  int
>  test_main (void)
>  {
> @@ -456,6 +486,7 @@ test_main (void)
>    test_init ();
> 
>    check1 ();
> +  check2 ();
> 
>    printf ("%23s", "");
>    FOR_EACH_IMPL (impl, 0)
> diff --git a/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> b/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> index bdd2ed2..f433683 100644
> --- a/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> +++ b/sysdeps/x86_64/multiarch/memcmp-ssse3.S
> @@ -1463,10 +1463,11 @@ L(next_24_bytes):
>         test    $0x40, %dh
>         jnz     L(Byte22)
> 
> -       mov     -9(%rdi), %eax
> -       and     $0xff, %eax
> -       mov     -9(%rsi), %edx
> -       and     $0xff, %edx
> +/* Movzbl reads only one byte and
> + doesn't read past the array boundary.  */

Make sure second line of comments line up to first.

> +
> +       movzbl  -9(%rdi), %eax
> +       movzbl  -9(%rsi), %edx
>         sub     %edx, %eax
>         ret
>  # else
> 
> 
> --
> Liubov
> 

OK with those fixes.

Cheers,
Carlos.