[PATCH] tst-safe-linking: make false positives even more improbable

[PATCH] tst-safe-linking: make false positives even more improbable

[Siddhesh Poyarekar]

There is a 1 in 16 chance of a corruption escaping safe-linking and to
guard against spurious failures, tst-safe-linking runs each subtest 10
times to ensure that the chance is reduced to 1 in 2^40.  However, in
the 1 in 16 chance that a corruption does escape safe linking, it
could well be caught by other sanity checks we do in malloc, which
then results in spurious test failures like below:

test test_fastbin_consolidate failed with a different error
  expected: malloc_consolidate(): unaligned fastbin chunk detected

  actual:   malloc_consolidate(): invalid chunk size

This failure is seen more frequently on i686; I was able to reproduce
it in about 5 min of running it in a loop.

Guard against such failures by recording them and retrying the test.
Also, do not fail the test if we happened to get defeated by the 1 in
2^40 odds if in at least one of the instances it was detected by other
checks.

Finally, bolster the odds to 2^64 by running 16 times instead of 10.
The test still has a chance (about 1 in 2^40) of failure so it is
still flaky in theory.  However in practice if we see a failure here
then it's more likely that there's a bug than it being an issue with
the test.  Add more printfs and also dump them to stdout so that in
the event the test actually fails, we will have some data to try and
understand why it may have failed.
---
 malloc/tst-safe-linking.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/malloc/tst-safe-linking.c b/malloc/tst-safe-linking.c
index 97cc108be6..959ba59704 100644
--- a/malloc/tst-safe-linking.c
+++ b/malloc/tst-safe-linking.c
@@ -33,32 +33,39 @@ check (const char *test, void (*callback) (void *),
        const char *expected)
 {
   int i, rand_mask;
-  bool success = false;
+  int success = 0;	/* 0 == fail, 1 == other check 2 == safe linking */
   /* There is a chance of 1/16 that a corrupted pointer will be aligned.
      Try multiple times so that statistical failure will be improbable.  */
-  for (i = 0; i < 10 && !success; ++i)
+  for (i = 0; i < 16; ++i)
     {
       rand_mask = rand () & 0xFF;
       struct support_capture_subprocess result
 	= support_capture_subprocess (callback, &rand_mask);
+      printf ("%s\n", result.out.buffer);
       /* Did not crash, could happen.  Try again.  */
       if (strlen (result.err.buffer) == 0)
 	continue;
-      /* Crashed, must be the expected result.  */
+      /* Crashed, it may either be safe linking or some other check.  If it's
+	 not safe linking then try again.  */
       if (strcmp (result.err.buffer, expected) != 0)
 	{
-	  support_record_failure ();
-	  printf ("error: test %s unexpected standard error data\n"
+	  printf ("test %s failed with a different error\n"
 	          "  expected: %s\n"
 	          "  actual:   %s\n",
 	          test, expected, result.err.buffer);
+	  success = 1;
+	  continue;
 	}
       TEST_VERIFY (WIFSIGNALED (result.status));
       if (WIFSIGNALED (result.status))
 	TEST_VERIFY (WTERMSIG (result.status) == SIGABRT);
       support_capture_subprocess_free (&result);
-      success = true;
+      success = 2;
+      break;
     }
+  /* The test fails only if the corruption was not caught by any of the malloc
+     mechanisms in all those iterations.  This has a lower than 1 in 2^64
+     chance of a false positive.  */
   TEST_VERIFY (success);
 }
 
@@ -74,10 +81,13 @@ test_tcache (void *closure)
   int mask = ((int *)closure)[0];
   size_t size = TCACHE_ALLOC_SIZE;
 
+  printf ("++ tcache ++\n");
+
   /* Populate the tcache list.  */
   void * volatile a = malloc (size);
   void * volatile b = malloc (size);
   void * volatile c = malloc (size);
+  printf ("a=%p, b=%p, c=%p\n", a, b, c);
   free (a);
   free (b);
   free (c);
@@ -88,6 +98,7 @@ test_tcache (void *closure)
   printf ("After: c=%p, c[0]=%p\n", c, ((void **)c)[0]);
 
   c = malloc (size);
+  printf ("Allocated: c=%p\n", c);
   /* This line will trigger the Safe-Linking check.  */
   b = malloc (size);
   printf ("b=%p\n", b);
@@ -101,10 +112,13 @@ test_fastbin (void *closure)
   int mask = ((int *)closure)[0];
   size_t size = TCACHE_ALLOC_SIZE;
 
+  printf ("++ fastbin ++\n");
+
   /* Take the tcache out of the game.  */
   for (i = 0; i < TCACHE_FILL_COUNT; ++i)
     {
       void * volatile p = calloc (1, size);
+      printf ("p=%p\n", p);
       free (p);
     }
 
@@ -112,6 +126,7 @@ test_fastbin (void *closure)
   void * volatile a = calloc (1, size);
   void * volatile b = calloc (1, size);
   void * volatile c = calloc (1, size);
+  printf ("a=%p, b=%p, c=%p\n", a, b, c);
   free (a);
   free (b);
   free (c);
@@ -122,6 +137,7 @@ test_fastbin (void *closure)
   printf ("After: c=%p, c[0]=%p\n", c, ((void **)c)[0]);
 
   c = calloc (1, size);
+  printf ("Allocated: c=%p\n", c);
   /* This line will trigger the Safe-Linking check.  */
   b = calloc (1, size);
   printf ("b=%p\n", b);
@@ -135,6 +151,8 @@ test_fastbin_consolidate (void *closure)
   int mask = ((int*)closure)[0];
   size_t size = TCACHE_ALLOC_SIZE;
 
+  printf ("++ fastbin consolidate ++\n");
+
   /* Take the tcache out of the game.  */
   for (i = 0; i < TCACHE_FILL_COUNT; ++i)
     {
@@ -146,6 +164,7 @@ test_fastbin_consolidate (void *closure)
   void * volatile a = calloc (1, size);
   void * volatile b = calloc (1, size);
   void * volatile c = calloc (1, size);
+  printf ("a=%p, b=%p, c=%p\n", a, b, c);
   free (a);
   free (b);
   free (c);
-- 
2.31.1

Re: [PATCH] tst-safe-linking: make false positives even more improbable

[Carlos O'Donell]

On 7/12/21 11:34 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> There is a 1 in 16 chance of a corruption escaping safe-linking and to
> guard against spurious failures, tst-safe-linking runs each subtest 10
> times to ensure that the chance is reduced to 1 in 2^40.  However, in
> the 1 in 16 chance that a corruption does escape safe linking, it
> could well be caught by other sanity checks we do in malloc, which
> then results in spurious test failures like below:
> 
> test test_fastbin_consolidate failed with a different error
>   expected: malloc_consolidate(): unaligned fastbin chunk detected
> 
>   actual:   malloc_consolidate(): invalid chunk size
> 
> This failure is seen more frequently on i686; I was able to reproduce
> it in about 5 min of running it in a loop.
> 
> Guard against such failures by recording them and retrying the test.
> Also, do not fail the test if we happened to get defeated by the 1 in
> 2^40 odds if in at least one of the instances it was detected by other
> checks.
> 
> Finally, bolster the odds to 2^64 by running 16 times instead of 10.
> The test still has a chance (about 1 in 2^40) of failure so it is
> still flaky in theory.  However in practice if we see a failure here
> then it's more likely that there's a bug than it being an issue with
> the test.  Add more printfs and also dump them to stdout so that in
> the event the test actually fails, we will have some data to try and
> understand why it may have failed.

OK for 2.34.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  malloc/tst-safe-linking.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/malloc/tst-safe-linking.c b/malloc/tst-safe-linking.c
> index 97cc108be6..959ba59704 100644
> --- a/malloc/tst-safe-linking.c
> +++ b/malloc/tst-safe-linking.c
> @@ -33,32 +33,39 @@ check (const char *test, void (*callback) (void *),
>         const char *expected)
>  {
>    int i, rand_mask;
> -  bool success = false;
> +  int success = 0;	/* 0 == fail, 1 == other check 2 == safe linking */

OK.

>    /* There is a chance of 1/16 that a corrupted pointer will be aligned.
>       Try multiple times so that statistical failure will be improbable.  */
> -  for (i = 0; i < 10 && !success; ++i)
> +  for (i = 0; i < 16; ++i)

OK. Iterate a little more.

>      {
>        rand_mask = rand () & 0xFF;
>        struct support_capture_subprocess result
>  	= support_capture_subprocess (callback, &rand_mask);
> +      printf ("%s\n", result.out.buffer);

OK. A little more verbose.

>        /* Did not crash, could happen.  Try again.  */
>        if (strlen (result.err.buffer) == 0)
>  	continue;
> -      /* Crashed, must be the expected result.  */
> +      /* Crashed, it may either be safe linking or some other check.  If it's
> +	 not safe linking then try again.  */
>        if (strcmp (result.err.buffer, expected) != 0)
>  	{
> -	  support_record_failure ();

OK. Don't record a failure.

> -	  printf ("error: test %s unexpected standard error data\n"
> +	  printf ("test %s failed with a different error\n"
>  	          "  expected: %s\n"
>  	          "  actual:   %s\n",
>  	          test, expected, result.err.buffer);
> +	  success = 1;

OK. Some other malloc check caught the failure.

> +	  continue;
>  	}
>        TEST_VERIFY (WIFSIGNALED (result.status));
>        if (WIFSIGNALED (result.status))
>  	TEST_VERIFY (WTERMSIG (result.status) == SIGABRT);
>        support_capture_subprocess_free (&result);
> -      success = true;
> +      success = 2;

OK. safe-linking caught the failure.

> +      break;
>      }
> +  /* The test fails only if the corruption was not caught by any of the malloc
> +     mechanisms in all those iterations.  This has a lower than 1 in 2^64
> +     chance of a false positive.  */
>    TEST_VERIFY (success);
>  }
>  
> @@ -74,10 +81,13 @@ test_tcache (void *closure)
>    int mask = ((int *)closure)[0];
>    size_t size = TCACHE_ALLOC_SIZE;
>  
> +  printf ("++ tcache ++\n");
> +
>    /* Populate the tcache list.  */
>    void * volatile a = malloc (size);
>    void * volatile b = malloc (size);
>    void * volatile c = malloc (size);
> +  printf ("a=%p, b=%p, c=%p\n", a, b, c);
>    free (a);
>    free (b);
>    free (c);
> @@ -88,6 +98,7 @@ test_tcache (void *closure)
>    printf ("After: c=%p, c[0]=%p\n", c, ((void **)c)[0]);
>  
>    c = malloc (size);
> +  printf ("Allocated: c=%p\n", c);
>    /* This line will trigger the Safe-Linking check.  */
>    b = malloc (size);
>    printf ("b=%p\n", b);
> @@ -101,10 +112,13 @@ test_fastbin (void *closure)
>    int mask = ((int *)closure)[0];
>    size_t size = TCACHE_ALLOC_SIZE;
>  
> +  printf ("++ fastbin ++\n");
> +
>    /* Take the tcache out of the game.  */
>    for (i = 0; i < TCACHE_FILL_COUNT; ++i)
>      {
>        void * volatile p = calloc (1, size);
> +      printf ("p=%p\n", p);
>        free (p);
>      }
>  
> @@ -112,6 +126,7 @@ test_fastbin (void *closure)
>    void * volatile a = calloc (1, size);
>    void * volatile b = calloc (1, size);
>    void * volatile c = calloc (1, size);
> +  printf ("a=%p, b=%p, c=%p\n", a, b, c);
>    free (a);
>    free (b);
>    free (c);
> @@ -122,6 +137,7 @@ test_fastbin (void *closure)
>    printf ("After: c=%p, c[0]=%p\n", c, ((void **)c)[0]);
>  
>    c = calloc (1, size);
> +  printf ("Allocated: c=%p\n", c);
>    /* This line will trigger the Safe-Linking check.  */
>    b = calloc (1, size);
>    printf ("b=%p\n", b);
> @@ -135,6 +151,8 @@ test_fastbin_consolidate (void *closure)
>    int mask = ((int*)closure)[0];
>    size_t size = TCACHE_ALLOC_SIZE;
>  
> +  printf ("++ fastbin consolidate ++\n");
> +
>    /* Take the tcache out of the game.  */
>    for (i = 0; i < TCACHE_FILL_COUNT; ++i)
>      {
> @@ -146,6 +164,7 @@ test_fastbin_consolidate (void *closure)
>    void * volatile a = calloc (1, size);
>    void * volatile b = calloc (1, size);
>    void * volatile c = calloc (1, size);
> +  printf ("a=%p, b=%p, c=%p\n", a, b, c);

OK.

>    free (a);
>    free (b);
>    free (c);
> 


-- 
Cheers,
Carlos.