Re: tunables signed/unsigned bug & patch

[Carlos O'Donell <carlos at redhat dot com>]

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:
> 


-- 
Cheers,
Carlos.