[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.