[patch] tcache double free check

[Florian Weimer]

* DJ Delorie:

> diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
> index 33574faab6..96bf925333 100644
> --- a/dlfcn/dlerror.c
> +++ b/dlfcn/dlerror.c
> @@ -198,7 +198,10 @@ check_free (struct dl_action_result *rec)
>        Dl_info info;
>        if (_dl_addr (check_free, &info, &map, NULL) != 0 && map->l_ns == 0)
>  #endif
> -	free ((char *) rec->errstring);
> +	{
> +	  free ((char *) rec->errstring);
> +	  rec->errstring = NULL;
> +	}
>      }
>  }

This should go into a separate commit.  I'm not sure if this is the
right fix.  Why is check_free called twice?

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 67cdfd0ad2..beaab29a05 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2888,6 +2888,8 @@ mremap_chunk (mchunkptr p, size_t new_size)
>  typedef struct tcache_entry
>  {
>    struct tcache_entry *next;
> +  /* This field exists to detect double frees.  */
> +  struct tcache_perthread_struct *key;
>  } tcache_entry;
>  
>  /* There is one of these for each thread, which contains the
> @@ -2911,6 +2913,27 @@ tcache_put (mchunkptr chunk, size_t tc_idx)
>  {
>    tcache_entry *e = (tcache_entry *) chunk2mem (chunk);
>    assert (tc_idx < TCACHE_MAX_BINS);
> +
> +  /* This test succeeds on double free.  However, we don't 100% trust
> +     it, so verify it's not an unlikely coincidence before
> +     aborting.  */
> +  if (__glibc_unlikely (e->key == tcache))
> +    {
> +      tcache_entry *tmp;
> +      for (tmp = tcache->entries[tc_idx];
> +	   tmp;
> +	   tmp = tmp->next)
> +	{
> +	if (tmp == e)
> +	  malloc_printerr ("free(): double free detected in tcache");
> +	}
> +      /* If we get here, it was a coincidence.  We've wasted a few
> +	 cycles, but don't abort.  */
> +    }

Should the above land in a separate function?  The code below looks
pretty similar.

> +  /* Now we mark this chunk as "in the tcache" so the above test will
> +     detect a double free.  */
> +  e->key = tcache;

Does this put the address of the tcache control block on the heap?

>    e->next = tcache->entries[tc_idx];
>    tcache->entries[tc_idx] = e;
>    ++(tcache->counts[tc_idx]);
> @@ -2926,6 +2949,7 @@ tcache_get (size_t tc_idx)
>    assert (tcache->entries[tc_idx] > 0);
>    tcache->entries[tc_idx] = e->next;
>    --(tcache->counts[tc_idx]);
> +  e->key = NULL;
>    return (void *) e;
>  }

And this clears it again, so that it does not leak to user code, and
it's not necessary to check the entire list?  This should be mentioned
in a comment.

> +  free (x); // puts in tcache
> +  free (x); // should abort
> +
> +  printf("FAIL: tcache double free not detected\n");
> +  return 1;
> +}
> +
> +#define TEST_FUNCTION do_test
> +#define EXPECTED_SIGNAL SIGABRT
> +#include <support/test-driver.c>

This will print the malloc error message to the build output, where it
will confuse QE.  Perhaps you can use support_capture_subprocess to
capture the error message?

Thanks,
Florian