tunables signed/unsigned bug & patch

The range check for size_t tunables was checking against
(signed)(0xfff...fff), which is -1, so never passed (val>0 && val<-1).
This means half the existing tunables would never work :-(

I couldn't think of a clean way to handle both signed and unsigned in
the same function, so I split it into separate functions.

As an aside, tunables_strtoul() parses signed values but returns
unsigned values.  Ideally that would be split out too, but that's a
lot more code duplication.

DJ

On Thursday 19 January 2017 11:52 PM, DJ Delorie wrote:
> The range check for size_t tunables was checking against
> (signed)(0xfff...fff), which is -1, so never passed (val>0 && val<-1).
> This means half the existing tunables would never work :-(
> 
> I couldn't think of a clean way to handle both signed and unsigned in
> the same function, so I split it into separate functions.
> 
> As an aside, tunables_strtoul() parses signed values but returns
> unsigned values.  Ideally that would be split out too, but that's a
> lot more code duplication.

That looks OK, but please add a ChangeLog next time.

Thanks,
Siddhesh

On 01/19/2017 01:22 PM, DJ Delorie wrote:
> The range check for size_t tunables was checking against
> (signed)(0xfff...fff), which is -1, so never passed (val>0 && val<-1).
> This means half the existing tunables would never work :-(
> 
> I couldn't think of a clean way to handle both signed and unsigned in
> the same function, so I split it into separate functions.
> 
> As an aside, tunables_strtoul() parses signed values but returns
> unsigned values.  Ideally that would be split out too, but that's a
> lot more code duplication.
> 
> DJ

As Siddhesh noted this needs a changelog.

This also IMO needs a regression test.

e.g.

- Create test malloc/tst-malloc-tunables.c
  - Test calls mallopt to verify internal malloc parameter
    was set to non-default size_t value using tunable.
- Add test to malloc/Makefile.
- Set tunable with 'tst-malloc-tunables-ENV = GLIBC_TUNABLES=glibc.malloc.foo=x'

The only test we have now verifies glibc.malloc.check was set, but
it's not a size_t type.

Lastly please state the machines you tested this on e.g. x86_64
or others.

> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index e0119d1..c2f487a 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -172,10 +172,10 @@ tunables_strtoul (const char *nptr)
>     explicit constraints of the tunable or with the implicit constraints of its
>     type.  */
>  static void
> -tunable_set_val_if_valid_range (tunable_t *cur, const char *strval,
> +tunable_set_val_if_valid_range_signed (tunable_t *cur, const char *strval,
>  				int64_t default_min, int64_t default_max)
>  {
> -  int64_t val = tunables_strtoul (strval);
> +  int64_t val = (int64_t) tunables_strtoul (strval);

OK.

>  
>    int64_t min = cur->type.min;
>    int64_t max = cur->type.max;
> @@ -193,6 +193,28 @@ tunable_set_val_if_valid_range (tunable_t *cur, const char *strval,
>      }
>  }
>  
> +static void
> +tunable_set_val_if_valid_range_unsigned (tunable_t *cur, const char *strval,
> +					 uint64_t default_min, uint64_t default_max)
> +{
> +  uint64_t val = (uint64_t) tunables_strtoul (strval);

OK.

> +
> +  uint64_t min = cur->type.min;
> +  uint64_t max = cur->type.max;
> +
> +  if (min == max)
> +    {
> +      min = default_min;
> +      max = default_max;
> +    }
> +
> +  if (val >= min && val <= max)
> +    {
> +      cur->val.numval = val;
> +      cur->strval = strval;
> +    }
> +}
> +
>  /* Validate range of the input value and initialize the tunable CUR if it looks
>     good.  */
>  static void
> @@ -202,12 +224,12 @@ tunable_initialize (tunable_t *cur, const char *strval)
>      {
>      case TUNABLE_TYPE_INT_32:
>  	{
> -	  tunable_set_val_if_valid_range (cur, strval, INT32_MIN, INT32_MAX);
> +	  tunable_set_val_if_valid_range_signed (cur, strval, INT32_MIN, INT32_MAX);

OK. int32_t is signed.

>  	  break;
>  	}
>      case TUNABLE_TYPE_SIZE_T:
>  	{
> -	  tunable_set_val_if_valid_range (cur, strval, 0, SIZE_MAX);
> +	  tunable_set_val_if_valid_range_unsigned (cur, strval, 0, SIZE_MAX);

OK. size_t is unsigned.

>  	  break;
>  	}
>      case TUNABLE_TYPE_STRING:
>

On 01/19/2017 02:37 PM, Carlos O'Donell wrote:
> This also IMO needs a regression test.
> 
> e.g.
> 
> - Create test malloc/tst-malloc-tunables.c
>   - Test calls mallopt to verify internal malloc parameter
>     was set to non-default size_t value using tunable.

For some reason I thought mallopt would return the old value
via the return, but it clearly doesn't. My mistake.

> - Add test to malloc/Makefile.
> - Set tunable with 'tst-malloc-tunables-ENV = GLIBC_TUNABLES=glibc.malloc.foo=x'
 
So the only way you can verify tunables is by indirect observation.

The easiest to test is MALLOC_ARENA_MAX which is a SIZE_T.

Set it to 1 (a value which will never result from even a 1 CPU system).

Spawn one thread per cpu and have them call malloc free in a tight loop.

Once all threads are running call malloc_info to count <heap> elements
(arenas).

If heaps > 1, then FAIL the test, clearly we didn't set arena max.

If heaps == 1, then it could be a false negative, the test has failed
to set arena max, and the threads managed to avoid each other such that
a second heap was never created (vanishingly unlikely).

So this gives you a relatively robust test that will very likely catch
the problem we saw.

The only _real_ way to fix this is to expose a GLIBC_PRIVATE symbols
which allow access to get/set the tunables.