tunables signed/unsigned bug & patch
Commit Message
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
Comments
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.
@@ -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);
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);
+
+ 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);
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);
break;
}
case TUNABLE_TYPE_STRING: