Fix tcache count maximum

  There are 2 issues with tcache count: the tcache counts[] array
is a char, which may be signed and has a very small range and thus
may overflow.  When setting it, there is no overflow check.

Eg. export GLIBC_TUNABLES=glibc.malloc.tcache_count=4096
leads to various crashes in benchtests:

Running /build/glibc/benchtests/bench-strcoll
bench-strcoll: malloc.c:2949: tcache_get: Assertion `tcache->counts[tc_idx] > 0' failed.
Aborted

Btw is this kind of crash regarded as a security risk? It's easy to do a
denial of service this way if you're able to set an environment variable.

ChangeLog:
2019-04-15  Wilco Dijkstra  <wdijkstr@arm.com>

        * malloc/malloc.c (tcache_perthread_struct): Use an
        unsigned short for counts array.
        (MAX_TCACHE_COUNT): New define.
        (do_set_tcache_count): Only update if count is small enough.

--
  

On 4/15/19 10:32 AM, Wilco Dijkstra wrote:
> There are 2 issues with tcache count: the tcache counts[] array
> is a char, which may be signed and has a very small range and thus
> may overflow.  When setting it, there is no overflow check.
> 
> Eg. export GLIBC_TUNABLES=glibc.malloc.tcache_count=4096
> leads to various crashes in benchtests:
> 
> Running /build/glibc/benchtests/bench-strcoll
> bench-strcoll: malloc.c:2949: tcache_get: Assertion `tcache->counts[tc_idx] > 0' failed.
> Aborted
> 
> Btw is this kind of crash regarded as a security risk? It's easy to do a
> denial of service this way if you're able to set an environment variable.

I don't think this is a security bug. If you control the environment variables
going into a non-SUID application then you can use LD_PRELOAD to load any other
library you have on the system, or take control of a lot of other glibc options.

> ChangeLog:
> 2019-04-15  Wilco Dijkstra  <wdijkstr@arm.com>
> 

Please create a bug for this.

This is a publicly visible issue with tcache and tunables.

>          * malloc/malloc.c (tcache_perthread_struct): Use an
>          unsigned short for counts array.
>          (MAX_TCACHE_COUNT): New define.
>          (do_set_tcache_count): Only update if count is small enough.
> 
> --
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 801ba1f499b566e677b763fc84f8ba86f4f7ccd0..4db7283cc27118cd7d39410febf7be8f7633780a 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2915,10 +2915,12 @@ typedef struct tcache_entry
>      time), this is for performance reasons.  */
>   typedef struct tcache_perthread_struct
>   {
> -  char counts[TCACHE_MAX_BINS];
> +  unsigned short counts[TCACHE_MAX_BINS];

This patch conflates two issues.

(a) Adding checking to tunable.

(b) Lifting limit.

Please split into two bugs. One to fix tunables, another to raise the
tcache chunk caching limit.

If you are going to do (b) and change the sign of counts then you need
to go through and fix other code that expects to possibly have a
negative value.

e.g.

2925 /* Caller must ensure that we know tc_idx is valid and there's room
2926    for more chunks.  */
2927 static __always_inline void
2928 tcache_put (mchunkptr chunk, size_t tc_idx)
2929 {
2930   tcache_entry *e = (tcache_entry *) chunk2mem (chunk);
2931   assert (tc_idx < TCACHE_MAX_BINS);
2932
2933   /* Mark this chunk as "in the tcache" so the test in _int_free will
2934      detect a double free.  */
2935   e->key = tcache;
2936
2937   e->next = tcache->entries[tc_idx];
2938   tcache->entries[tc_idx] = e;
2939   ++(tcache->counts[tc_idx]);

        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
        assert (tcache->counts[tc_idx] != 0);
        See below for discussion.

2940 }


2942 /* Caller must ensure that we know tc_idx is valid and there's
2943    available chunks to remove.  */
2944 static __always_inline void *
2945 tcache_get (size_t tc_idx)
2946 {
2947   tcache_entry *e = tcache->entries[tc_idx];
2948   assert (tc_idx < TCACHE_MAX_BINS);
2949   assert (tcache->counts[tc_idx] > 0);

        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        Always true now if counts is only positive.
        Remove?

2950   tcache->entries[tc_idx] = e->next;
2951   --(tcache->counts[tc_idx]);

        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
        May wrap on error, should we check that and assert?
        We expect the caller to check for != NULL entry,
        indicating at least one entry. It's possible the list
        is corrupt and 'e' is pointing to garbage, so an
        assert might be good here?

        assert (tcache->counts[tc_idx] != MAX_TCACHE_COUNT);

2952   e->key = NULL;
2953   return (void *) e;
2954 }

The manual/memory.texi needs updating because you made the
count twice the size, and the rough estimates for size of
tcache should be updated. The manual should also list the
actual limit being imposed.

>     tcache_entry *entries[TCACHE_MAX_BINS];
>   } tcache_perthread_struct;
>   
> +#define MAX_TCACHE_COUNT 65535	/* Maximum value of counts[] entries.  */

OK. But see notes above about a new bug, distinct patch, and
manual update.

> +
>   static __thread bool tcache_shutting_down = false;
>   static __thread tcache_perthread_struct *tcache = NULL;
>   
> @@ -5114,8 +5116,11 @@ do_set_tcache_max (size_t value)
>   static __always_inline int
>   do_set_tcache_count (size_t value)
>   {
> -  LIBC_PROBE (memory_tunable_tcache_count, 2, value, mp_.tcache_count);
> -  mp_.tcache_count = value;
> +  if (value <= MAX_TCACHE_COUNT)
> +    {
> +      LIBC_PROBE (memory_tunable_tcache_count, 2, value, mp_.tcache_count);
> +      mp_.tcache_count = value;

OK.

> +    }
>     return 1;
>   }
>   
>