diff mbox

[2/4] Initialize tunable list with the GLIBC_TUNABLES environment variable

Message ID 1479285306-11684-3-git-send-email-siddhesh@sourceware.org
State New, archived
Headers

Commit Message

Siddhesh Poyarekar Nov. 16, 2016, 8:35 a.m. UTC 
  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

	* elf/dl-tunables.c: Include mman.h.
	(GLIBC_TUNABLES): New macro.
	(tunables_strdup): New function.
	(parse_tunables): New function.
	(min_strlen): New function.
	(disable_tunable): Use the new functions and macro.
	(__tunables_init): Likewise.
	(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.
---
 elf/dl-tunables.c                          | 129 ++++++++++++++++++++++++++++-
 malloc/Makefile                            |   6 +-
 malloc/tst-malloc-usable-static-tunables.c |   1 +
 malloc/tst-malloc-usable-tunables.c        |   1 +
 4 files changed, 135 insertions(+), 2 deletions(-)
 create mode 100644 malloc/tst-malloc-usable-static-tunables.c
 create mode 100644 malloc/tst-malloc-usable-tunables.c

Comments

Florian Weimer Dec. 27, 2016, 11:41 a.m. UTC | #1 
On 11/16/2016 09:35 AM, Siddhesh Poyarekar wrote:

> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c

> +static char *
> +tunables_strdup (const char *in)
> +{
> +  size_t i = 0;
> +
> +  while (in[i++]);

Please use an explicit comparison against '\0'.

> +  char *out = __mmap (NULL, ALIGN_UP (i, __getpagesize ()),
> +		      PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1,
> +		      0);
> +
> +  if (out == MAP_FAILED)
> +    return NULL;

See my initial mail.  errno and __getpagesize are not available at this 
point.

> +static size_t
> +min_strlen (const char *s)
> +{
> +  size_t i = 0;
> +  while (*s++ != '\0')
> +    i++;
> +
> +  return i;
> +}

Is there anything which ensures that GCC does not replace this 
implementation with strlen?

>  /* Disable a tunable if it is set.  */
>  static void
>  disable_tunable (tunable_id_t id, char **envp)
> @@ -216,6 +318,23 @@ disable_tunable (tunable_id_t id, char **envp)
>
>    if (env_alias)
>      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++;
> +    }
>  }

This assumes that the process environment is not mapped read-only.  I'm 
not sure if this is guaranteed by the ABI.

Thanks,
Florian
Siddhesh Poyarekar Dec. 27, 2016, 9:35 p.m. UTC | #2 
On Tuesday 27 December 2016 05:11 PM, Florian Weimer wrote:
> See my initial mail.  errno and __getpagesize are not available at this
> point.

Ugh, right.  I'll try to come up with something that does not need an
additional allocation.

>> +static size_t
>> +min_strlen (const char *s)
>> +{
>> +  size_t i = 0;
>> +  while (*s++ != '\0')
>> +    i++;
>> +
>> +  return i;
>> +}
> 
> Is there anything which ensures that GCC does not replace this
> implementation with strlen?

I think I'm missing a gcc flag here to ensure that.  I'll find out and
add it.

>>  /* Disable a tunable if it is set.  */
>>  static void
>>  disable_tunable (tunable_id_t id, char **envp)
>> @@ -216,6 +318,23 @@ disable_tunable (tunable_id_t id, char **envp)
>>
>>    if (env_alias)
>>      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++;
>> +    }
>>  }
> 
> This assumes that the process environment is not mapped read-only.  I'm
> not sure if this is guaranteed by the ABI.

Barring tunables with string values this might be easy to work around.
Let me give this a shot too.

Siddhesh
diff mbox

Patch

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 91340b6..79ec8ef 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -22,6 +22,7 @@ 
 #include <stdbool.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <sys/mman.h>
 #include <libc-internal.h>
 #include <sysdep.h>
 #include <fcntl.h>
@@ -29,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)
 {
@@ -44,6 +48,28 @@  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 = __mmap (NULL, ALIGN_UP (i, __getpagesize ()),
+		      PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1,
+		      0);
+
+  if (out == MAP_FAILED)
+    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)
 {
@@ -208,6 +234,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)
@@ -216,6 +318,23 @@  disable_tunable (tunable_id_t id, char **envp)
 
   if (env_alias)
     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
@@ -246,6 +365,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];
diff --git a/malloc/Makefile b/malloc/Makefile
index 4e4104e..a34e20c 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -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
diff --git a/malloc/tst-malloc-usable-static-tunables.c b/malloc/tst-malloc-usable-static-tunables.c
new file mode 100644
index 0000000..8907db0
--- /dev/null
+++ b/malloc/tst-malloc-usable-static-tunables.c
@@ -0,0 +1 @@ 
+#include <malloc/tst-malloc-usable.c>
diff --git a/malloc/tst-malloc-usable-tunables.c b/malloc/tst-malloc-usable-tunables.c
new file mode 100644
index 0000000..8907db0
--- /dev/null
+++ b/malloc/tst-malloc-usable-tunables.c
@@ -0,0 +1 @@ 
+#include <malloc/tst-malloc-usable.c>