[2/4] Initialize tunable list with the GLIBC_TUNABLES environment variable
Commit Message
Read tunables values from the users using the GLIBC_TUNABLES
environment variable. The value of this variable is a colon-separated
list of name=value pairs. So a typical string would look like this:
GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024
* config.make.in (have-loop-to-function): Define.
* elf/Makefile (CFLAGS-dl-tunables.c): Add
-fno-tree-loop-distribute-patterns.
* elf/dl-tunables.c: Include libc-internals.h.
(GLIBC_TUNABLES): New macro.
(tunables_strdup): New function.
(parse_tunables): New function.
(min_strlen): New function.
(__tunables_init): Use the new functions and macro.
(disable_tunable): Disable tunable from GLIBC_TUNABLES.
* malloc/tst-malloc-usable-tunables.c: New test case.
* malloc/tst-malloc-usable-static-tunables.c: New test case.
* malloc/Makefile (tests, tests-static): Add tests.
---
config.make.in | 1 +
elf/Makefile | 6 ++
elf/dl-tunables.c | 129 ++++++++++++++++++++++++++++-
malloc/Makefile | 6 +-
malloc/tst-malloc-usable-static-tunables.c | 1 +
malloc/tst-malloc-usable-tunables.c | 1 +
6 files changed, 142 insertions(+), 2 deletions(-)
create mode 100644 malloc/tst-malloc-usable-static-tunables.c
create mode 100644 malloc/tst-malloc-usable-tunables.c
Comments
On 12/31/2016 04:40 PM, Siddhesh Poyarekar wrote:
> + while (in[i++]);
> + char *out = __sbrk (i);
This still has an implicit comparision against '\0', which is against
the style guide.
> + /* FIXME: In reality this will not return NULL, it will crash attempting to
> + set the thread-local errno since the TCB has not yet been set up. This
> + needs to be fixed with an __sbrk implementation that does not set
> + errno. */
> + if (out == (void *)-1)
> + return NULL;
Comment does not match: NULL is not (void *) -1.
Thanks,
Florian
On Saturday 31 December 2016 09:15 PM, Florian Weimer wrote:
> This still has an implicit comparision against '\0', which is against
> the style guide.
Ack, I'll fix it up.
>> + /* FIXME: In reality this will not return NULL, it will crash
>> attempting to
>> + set the thread-local errno since the TCB has not yet been set
>> up. This
>> + needs to be fixed with an __sbrk implementation that does not set
>> + errno. */
>> + if (out == (void *)-1)
>> + return NULL;
>
> Comment does not match: NULL is not (void *) -1.
I actually meant the NULL return of the function, not __sbrk. I could
put the comment on top of the function to make it clearer.
Any other issues with this patch? I could fix these up and push if
there's nothing else. That is of course also provided that 1/4 is good
to go.
Siddhesh
On Saturday 31 December 2016 09:20 PM, Siddhesh Poyarekar wrote:
> On Saturday 31 December 2016 09:15 PM, Florian Weimer wrote:
>> This still has an implicit comparision against '\0', which is against
>> the style guide.
>
> Ack, I'll fix it up.
>
>>> + /* FIXME: In reality this will not return NULL, it will crash
>>> attempting to
>>> + set the thread-local errno since the TCB has not yet been set
>>> up. This
>>> + needs to be fixed with an __sbrk implementation that does not set
>>> + errno. */
>>> + if (out == (void *)-1)
>>> + return NULL;
>>
>> Comment does not match: NULL is not (void *) -1.
I changed it to:
FIXME: In reality if the allocation fails, __sbrk will crash attempting
to set the thread-local errno since the TCB has not yet been set up.
This needs to be fixed with an __sbrk implementation that does not set
errno.
On 12/31/2016 04:55 PM, Siddhesh Poyarekar wrote:
>
>
> On Saturday 31 December 2016 09:20 PM, Siddhesh Poyarekar wrote:
>> On Saturday 31 December 2016 09:15 PM, Florian Weimer wrote:
>>> This still has an implicit comparision against '\0', which is against
>>> the style guide.
>>
>> Ack, I'll fix it up.
>>
>>>> + /* FIXME: In reality this will not return NULL, it will crash
>>>> attempting to
>>>> + set the thread-local errno since the TCB has not yet been set
>>>> up. This
>>>> + needs to be fixed with an __sbrk implementation that does not set
>>>> + errno. */
>>>> + if (out == (void *)-1)
>>>> + return NULL;
>>>
>>> Comment does not match: NULL is not (void *) -1.
>
> I changed it to:
>
>
> FIXME: In reality if the allocation fails, __sbrk will crash attempting
> to set the thread-local errno since the TCB has not yet been set up.
> This needs to be fixed with an __sbrk implementation that does not set
> errno.
Fine with me. Thanks.
Florian
@@ -71,6 +71,7 @@ have-hash-style = @libc_cv_hashstyle@
use-default-link = @use_default_link@
output-format = @libc_cv_output_format@
have-cxx-thread_local = @libc_cv_cxx_thread_local@
+have-loop-to-function = @libc_cv_cc_loop_to_function@
multi-arch = @multi_arch@
@@ -38,6 +38,12 @@ endif
ifeq (yes,$(have-tunables))
dl-routines += dl-tunables
+
+# Make sure that the compiler does not insert any library calls in tunables
+# code paths.
+ifeq (yes,$(have-loop-to-function))
+CFLAGS-dl-tunables.c = -fno-tree-loop-distribute-patterns
+endif
endif
all-dl-routines = $(dl-routines) $(sysdep-dl-routines)
@@ -30,7 +30,10 @@
#define TUNABLES_INTERNAL 1
#include "dl-tunables.h"
-/* Compare environment names, bounded by the name hardcoded in glibc. */
+#define GLIBC_TUNABLES "GLIBC_TUNABLES"
+
+/* Compare environment or tunable names, bounded by the name hardcoded in
+ glibc. */
static bool
is_name (const char *orig, const char *envname)
{
@@ -45,6 +48,29 @@ is_name (const char *orig, const char *envname)
return false;
}
+static char *
+tunables_strdup (const char *in)
+{
+ size_t i = 0;
+
+ while (in[i++]);
+ char *out = __sbrk (i);
+
+ /* FIXME: In reality this will not return NULL, it will crash attempting to
+ set the thread-local errno since the TCB has not yet been set up. This
+ needs to be fixed with an __sbrk implementation that does not set
+ errno. */
+ if (out == (void *)-1)
+ return NULL;
+
+ i--;
+
+ while (i-- > 0)
+ out[i] = in[i];
+
+ return out;
+}
+
static char **
get_next_env (char **envp, char **name, size_t *namelen, char **val)
{
@@ -218,6 +244,82 @@ tunable_initialize (tunable_t *cur, const char *strval)
}
}
+static void
+parse_tunables (char *tunestr)
+{
+ if (tunestr == NULL || *tunestr == '\0')
+ return;
+
+ char *p = tunestr;
+
+ while (true)
+ {
+ char *name = p;
+ size_t len = 0;
+
+ /* First, find where the name ends. */
+ while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
+ len++;
+
+ /* If we reach the end of the string before getting a valid name-value
+ pair, bail out. */
+ if (p[len] == '\0')
+ return;
+
+ /* We did not find a valid name-value pair before encountering the
+ colon. */
+ if (p[len]== ':')
+ {
+ p += len + 1;
+ continue;
+ }
+
+ p += len + 1;
+
+ char *value = p;
+ len = 0;
+
+ while (p[len] != ':' && p[len] != '\0')
+ len++;
+
+ char end = p[len];
+ p[len] = '\0';
+
+ /* Add the tunable if it exists. */
+ for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
+ {
+ tunable_t *cur = &tunable_list[i];
+
+ /* If we are in a secure context (AT_SECURE) then ignore the tunable
+ unless it is explicitly marked as secure. Tunable values take
+ precendence over their envvar aliases. */
+ if (__libc_enable_secure && !cur->is_secure)
+ continue;
+
+ if (is_name (cur->name, name))
+ {
+ tunable_initialize (cur, value);
+ break;
+ }
+ }
+
+ if (end == ':')
+ p += len + 1;
+ else
+ return;
+ }
+}
+
+static size_t
+min_strlen (const char *s)
+{
+ size_t i = 0;
+ while (*s++ != '\0')
+ i++;
+
+ return i;
+}
+
/* Disable a tunable if it is set. */
static void
disable_tunable (tunable_id_t id, char **envp)
@@ -226,6 +328,23 @@ disable_tunable (tunable_id_t id, char **envp)
if (env_alias != NULL)
tunables_unsetenv (envp, tunable_list[id].env_alias);
+
+ char *tunable = getenv (GLIBC_TUNABLES);
+ const char *cmp = tunable_list[id].name;
+ const size_t len = min_strlen (cmp);
+
+ while (tunable && *tunable != '\0' && *tunable != ':')
+ {
+ if (is_name (tunable, cmp))
+ {
+ tunable += len;
+ /* Overwrite the = and the value with colons. */
+ while (*tunable != '\0' && *tunable != ':')
+ *tunable++ = ':';
+ break;
+ }
+ tunable++;
+ }
}
/* Disable the glibc.malloc.check tunable in SETUID/SETGID programs unless
@@ -256,6 +375,14 @@ __tunables_init (char **envp)
while ((envp = get_next_env (envp, &envname, &len, &envval)) != NULL)
{
+ if (is_name (GLIBC_TUNABLES, envname))
+ {
+ char *val = tunables_strdup (envval);
+ if (val != NULL)
+ parse_tunables (val);
+ continue;
+ }
+
for (int i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
{
tunable_t *cur = &tunable_list[i];
@@ -33,11 +33,13 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
tst-mallocfork2 \
tst-interpose-nothread \
tst-interpose-thread \
+ tst-malloc-usable-tunables
tests-static := \
tst-interpose-static-nothread \
tst-interpose-static-thread \
- tst-malloc-usable-static
+ tst-malloc-usable-static \
+ tst-malloc-usable-static-tunables
tests += $(tests-static)
test-srcs = tst-mtrace
@@ -160,6 +162,8 @@ endif
tst-mcheck-ENV = MALLOC_CHECK_=3
tst-malloc-usable-ENV = MALLOC_CHECK_=3
tst-malloc-usable-static-ENV = $(tst-malloc-usable-ENV)
+tst-malloc-usable-tunables-ENV = GLIBC_TUNABLES=glibc.malloc.check=3
+tst-malloc-usable-static-tunables-ENV = $(tst-malloc-usable-tunables-ENV)
# Uncomment this for test releases. For public releases it is too expensive.
#CPPFLAGS-malloc.o += -DMALLOC_DEBUG=1
new file mode 100644
@@ -0,0 +1 @@
+#include <malloc/tst-malloc-usable.c>
new file mode 100644
@@ -0,0 +1 @@
+#include <malloc/tst-malloc-usable.c>