| Message ID | 20231122204325.4058222-3-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve loader environment variable handling | expand |
On 2023-11-22 15:43, Adhemerval Zanella wrote: > The loader now warns for invalid and out-of-range tunable values. The > patch also fixes the parsing of size_t maximum values, where > _dl_strtoul was failing for large values close to SIZE_MAX. > > Checked on x86_64-linux-gnu. > --- > elf/dl-misc.c | 5 ++++- > elf/dl-tunables.c | 35 ++++++++++++++++++++++++++++++----- > elf/tst-tunables.c | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 64 insertions(+), 6 deletions(-) > > diff --git a/elf/dl-misc.c b/elf/dl-misc.c > index 5b84adc2f4..037cbb3650 100644 > --- a/elf/dl-misc.c > +++ b/elf/dl-misc.c > @@ -190,6 +190,9 @@ _dl_strtoul (const char *nptr, char **endptr) > } > } > > + const uint64_t cutoff = (UINT64_MAX * 2UL + 1UL) / 10; > + const uint64_t cutlim = (UINT64_MAX * 2UL + 1UL) % 10; > + > while (1) > { > int digval; > @@ -207,7 +210,7 @@ _dl_strtoul (const char *nptr, char **endptr) > else > break; > > - if (result >= (UINT64_MAX - digval) / base) > + if (result > cutoff || (result == cutoff && digval > cutlim)) I don't understand this change; how does this work with octal or hexadecimal inputs? > { > if (endptr != NULL) > *endptr = (char *) nptr; > diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c > index 26161c68e7..67a37ff704 100644 > --- a/elf/dl-tunables.c > +++ b/elf/dl-tunables.c > @@ -77,16 +77,27 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp, > { > tunable_num_t val, min, max; > > - if (cur->type.type_code == TUNABLE_TYPE_STRING) > + switch (cur->type.type_code) > { > + case TUNABLE_TYPE_STRING: > cur->val.strval = valp->strval; > cur->initialized = true; > return; > + case TUNABLE_TYPE_INT_32: > + val = (int32_t) valp->numval; > + break; > + case TUNABLE_TYPE_UINT_64: > + val = (int64_t) valp->numval; > + break; > + case TUNABLE_TYPE_SIZE_T: > + val = (size_t) valp->numval; > + break; > + default: > + __builtin_unreachable (); > } > > bool unsigned_cmp = unsigned_tunable_type (cur->type.type_code); > > - val = valp->numval; > min = minp != NULL ? *minp : cur->type.min; > max = maxp != NULL ? *maxp : cur->type.max; > > @@ -117,16 +128,24 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp, > > /* Validate range of the input value and initialize the tunable CUR if it looks > good. */ > -static void > +static bool > tunable_initialize (tunable_t *cur, const char *strval, size_t len) > { > tunable_val_t val = { 0 }; > > if (cur->type.type_code != TUNABLE_TYPE_STRING) > - val.numval = (tunable_num_t) _dl_strtoul (strval, NULL); > + { > + char *endptr = NULL; > + uint64_t numval = _dl_strtoul (strval, &endptr); > + if (endptr != strval + len) > + return false; > + val.numval = (tunable_num_t) numval; > + } > else > val.strval = (struct tunable_str_t) { strval, len }; > do_tunable_update_val (cur, &val, NULL, NULL); > + > + return true; > } > > void > @@ -226,7 +245,13 @@ parse_tunables (const char *valstring) > } > > for (int i = 0; i < ntunables; i++) > - tunable_initialize (tunables[i].t, tunables[i].value, tunables[i].len); > + if (!tunable_initialize (tunables[i].t, tunables[i].value, > + tunables[i].len)) > + _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' " > + "for option `%s': ignored.\n", > + (int) tunables[i].len, > + tunables[i].value, > + tunables[i].t->name); > } > > /* Initialize the tunables list from the environment. For now we only use the > diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c > index 188345b070..d6a1e1b3ac 100644 > --- a/elf/tst-tunables.c > +++ b/elf/tst-tunables.c > @@ -53,6 +53,13 @@ static const struct test_t > 4096, > 0, > }, > + { > + "GLIBC_TUNABLES", > + "glibc.malloc.mmap_threshold=-1", > + 0, > + SIZE_MAX, > + 0, > + }, > /* Empty tunable are ignored. */ > { > "GLIBC_TUNABLES", > @@ -224,6 +231,29 @@ static const struct test_t > 0, > 0, > }, > + /* Invalid numbers are ignored. */ > + { > + "GLIBC_TUNABLES", > + "glibc.malloc.check=abc:glibc.malloc.mmap_threshold=4096", > + 0, > + 4096, > + 0, > + }, > + { > + "GLIBC_TUNABLES", > + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=abc", > + 2, > + 0, > + 0, > + }, > + { > + "GLIBC_TUNABLES", > + /* SIZE_MAX + 1 */ > + "glibc.malloc.mmap_threshold=18446744073709551616", > + 0, > + 0, > + 0, > + }, > /* Also check some tunable aliases. */ > { > "MALLOC_CHECK_",
On 01/12/23 12:32, Siddhesh Poyarekar wrote: > > > On 2023-11-22 15:43, Adhemerval Zanella wrote: >> The loader now warns for invalid and out-of-range tunable values. The >> patch also fixes the parsing of size_t maximum values, where >> _dl_strtoul was failing for large values close to SIZE_MAX. >> >> Checked on x86_64-linux-gnu. >> --- >> elf/dl-misc.c | 5 ++++- >> elf/dl-tunables.c | 35 ++++++++++++++++++++++++++++++----- >> elf/tst-tunables.c | 30 ++++++++++++++++++++++++++++++ >> 3 files changed, 64 insertions(+), 6 deletions(-) >> >> diff --git a/elf/dl-misc.c b/elf/dl-misc.c >> index 5b84adc2f4..037cbb3650 100644 >> --- a/elf/dl-misc.c >> +++ b/elf/dl-misc.c >> @@ -190,6 +190,9 @@ _dl_strtoul (const char *nptr, char **endptr) >> } >> } >> + const uint64_t cutoff = (UINT64_MAX * 2UL + 1UL) / 10; >> + const uint64_t cutlim = (UINT64_MAX * 2UL + 1UL) % 10; >> + >> while (1) >> { >> int digval; >> @@ -207,7 +210,7 @@ _dl_strtoul (const char *nptr, char **endptr) >> else >> break; >> - if (result >= (UINT64_MAX - digval) / base) >> + if (result > cutoff || (result == cutoff && digval > cutlim)) > > I don't understand this change; how does this work with octal or hexadecimal inputs? In fact the cutoff/cutlim should be adjusted when a different base is used, I will fix it. The logic here is similar to the stdlib/strtol_l.c:486. > >> { >> if (endptr != NULL) >> *endptr = (char *) nptr; >> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c >> index 26161c68e7..67a37ff704 100644 >> --- a/elf/dl-tunables.c >> +++ b/elf/dl-tunables.c >> @@ -77,16 +77,27 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp, >> { >> tunable_num_t val, min, max; >> - if (cur->type.type_code == TUNABLE_TYPE_STRING) >> + switch (cur->type.type_code) >> { >> + case TUNABLE_TYPE_STRING: >> cur->val.strval = valp->strval; >> cur->initialized = true; >> return; >> + case TUNABLE_TYPE_INT_32: >> + val = (int32_t) valp->numval; >> + break; >> + case TUNABLE_TYPE_UINT_64: >> + val = (int64_t) valp->numval; >> + break; >> + case TUNABLE_TYPE_SIZE_T: >> + val = (size_t) valp->numval; >> + break; >> + default: >> + __builtin_unreachable (); >> } >> bool unsigned_cmp = unsigned_tunable_type (cur->type.type_code); >> - val = valp->numval; >> min = minp != NULL ? *minp : cur->type.min; >> max = maxp != NULL ? *maxp : cur->type.max; >> @@ -117,16 +128,24 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp, >> /* Validate range of the input value and initialize the tunable CUR if it looks >> good. */ >> -static void >> +static bool >> tunable_initialize (tunable_t *cur, const char *strval, size_t len) >> { >> tunable_val_t val = { 0 }; >> if (cur->type.type_code != TUNABLE_TYPE_STRING) >> - val.numval = (tunable_num_t) _dl_strtoul (strval, NULL); >> + { >> + char *endptr = NULL; >> + uint64_t numval = _dl_strtoul (strval, &endptr); >> + if (endptr != strval + len) >> + return false; >> + val.numval = (tunable_num_t) numval; >> + } >> else >> val.strval = (struct tunable_str_t) { strval, len }; >> do_tunable_update_val (cur, &val, NULL, NULL); >> + >> + return true; >> } >> void >> @@ -226,7 +245,13 @@ parse_tunables (const char *valstring) >> } >> for (int i = 0; i < ntunables; i++) >> - tunable_initialize (tunables[i].t, tunables[i].value, tunables[i].len); >> + if (!tunable_initialize (tunables[i].t, tunables[i].value, >> + tunables[i].len)) >> + _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' " >> + "for option `%s': ignored.\n", >> + (int) tunables[i].len, >> + tunables[i].value, >> + tunables[i].t->name); >> } >> /* Initialize the tunables list from the environment. For now we only use the >> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c >> index 188345b070..d6a1e1b3ac 100644 >> --- a/elf/tst-tunables.c >> +++ b/elf/tst-tunables.c >> @@ -53,6 +53,13 @@ static const struct test_t >> 4096, >> 0, >> }, >> + { >> + "GLIBC_TUNABLES", >> + "glibc.malloc.mmap_threshold=-1", >> + 0, >> + SIZE_MAX, >> + 0, >> + }, >> /* Empty tunable are ignored. */ >> { >> "GLIBC_TUNABLES", >> @@ -224,6 +231,29 @@ static const struct test_t >> 0, >> 0, >> }, >> + /* Invalid numbers are ignored. */ >> + { >> + "GLIBC_TUNABLES", >> + "glibc.malloc.check=abc:glibc.malloc.mmap_threshold=4096", >> + 0, >> + 4096, >> + 0, >> + }, >> + { >> + "GLIBC_TUNABLES", >> + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=abc", >> + 2, >> + 0, >> + 0, >> + }, >> + { >> + "GLIBC_TUNABLES", >> + /* SIZE_MAX + 1 */ >> + "glibc.malloc.mmap_threshold=18446744073709551616", >> + 0, >> + 0, >> + 0, >> + }, >> /* Also check some tunable aliases. */ >> { >> "MALLOC_CHECK_",
diff --git a/elf/dl-misc.c b/elf/dl-misc.c index 5b84adc2f4..037cbb3650 100644 --- a/elf/dl-misc.c +++ b/elf/dl-misc.c @@ -190,6 +190,9 @@ _dl_strtoul (const char *nptr, char **endptr) } } + const uint64_t cutoff = (UINT64_MAX * 2UL + 1UL) / 10; + const uint64_t cutlim = (UINT64_MAX * 2UL + 1UL) % 10; + while (1) { int digval; @@ -207,7 +210,7 @@ _dl_strtoul (const char *nptr, char **endptr) else break; - if (result >= (UINT64_MAX - digval) / base) + if (result > cutoff || (result == cutoff && digval > cutlim)) { if (endptr != NULL) *endptr = (char *) nptr; diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c index 26161c68e7..67a37ff704 100644 --- a/elf/dl-tunables.c +++ b/elf/dl-tunables.c @@ -77,16 +77,27 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp, { tunable_num_t val, min, max; - if (cur->type.type_code == TUNABLE_TYPE_STRING) + switch (cur->type.type_code) { + case TUNABLE_TYPE_STRING: cur->val.strval = valp->strval; cur->initialized = true; return; + case TUNABLE_TYPE_INT_32: + val = (int32_t) valp->numval; + break; + case TUNABLE_TYPE_UINT_64: + val = (int64_t) valp->numval; + break; + case TUNABLE_TYPE_SIZE_T: + val = (size_t) valp->numval; + break; + default: + __builtin_unreachable (); } bool unsigned_cmp = unsigned_tunable_type (cur->type.type_code); - val = valp->numval; min = minp != NULL ? *minp : cur->type.min; max = maxp != NULL ? *maxp : cur->type.max; @@ -117,16 +128,24 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp, /* Validate range of the input value and initialize the tunable CUR if it looks good. */ -static void +static bool tunable_initialize (tunable_t *cur, const char *strval, size_t len) { tunable_val_t val = { 0 }; if (cur->type.type_code != TUNABLE_TYPE_STRING) - val.numval = (tunable_num_t) _dl_strtoul (strval, NULL); + { + char *endptr = NULL; + uint64_t numval = _dl_strtoul (strval, &endptr); + if (endptr != strval + len) + return false; + val.numval = (tunable_num_t) numval; + } else val.strval = (struct tunable_str_t) { strval, len }; do_tunable_update_val (cur, &val, NULL, NULL); + + return true; } void @@ -226,7 +245,13 @@ parse_tunables (const char *valstring) } for (int i = 0; i < ntunables; i++) - tunable_initialize (tunables[i].t, tunables[i].value, tunables[i].len); + if (!tunable_initialize (tunables[i].t, tunables[i].value, + tunables[i].len)) + _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' " + "for option `%s': ignored.\n", + (int) tunables[i].len, + tunables[i].value, + tunables[i].t->name); } /* Initialize the tunables list from the environment. For now we only use the diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c index 188345b070..d6a1e1b3ac 100644 --- a/elf/tst-tunables.c +++ b/elf/tst-tunables.c @@ -53,6 +53,13 @@ static const struct test_t 4096, 0, }, + { + "GLIBC_TUNABLES", + "glibc.malloc.mmap_threshold=-1", + 0, + SIZE_MAX, + 0, + }, /* Empty tunable are ignored. */ { "GLIBC_TUNABLES", @@ -224,6 +231,29 @@ static const struct test_t 0, 0, }, + /* Invalid numbers are ignored. */ + { + "GLIBC_TUNABLES", + "glibc.malloc.check=abc:glibc.malloc.mmap_threshold=4096", + 0, + 4096, + 0, + }, + { + "GLIBC_TUNABLES", + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=abc", + 2, + 0, + 0, + }, + { + "GLIBC_TUNABLES", + /* SIZE_MAX + 1 */ + "glibc.malloc.mmap_threshold=18446744073709551616", + 0, + 0, + 0, + }, /* Also check some tunable aliases. */ { "MALLOC_CHECK_",