public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix range check in do_tunable_update_val
@ 2017-07-19 21:34 Alexey Makhalov
  2017-07-20  1:43 ` Carlos O'Donell
  0 siblings, 1 reply; 6+ messages in thread
From: Alexey Makhalov @ 2017-07-19 21:34 UTC (permalink / raw)
  To: libc-alpha; +Cc: amakhalov

Current implementation of tunables does not set arena_max and arena_test
values. Any value provided by glibc.malloc.arena_max and
glibc.malloc.arena_test parameters is ignored.

These tunables have minval value set to 1 (see elf/dl-tunables.list file)
and undefined maxval value. In that case default value (which is 0. see
scripts/gen-tunables.awk) is being used to set maxval.

For instance, generated tunable_list[] entry for arena_max is:
(gdb) p *cur
$1 = {name = 0x7ffff7df6217 "glibc.malloc.arena_max",
 type = {type_code = TUNABLE_TYPE_SIZE_T, min = 1, max = 0},
  val = {numval = 0, strval = 0x0}, initialized = false,
   security_level = TUNABLE_SECLEVEL_SXID_IGNORE,
    env_alias = 0x7ffff7df622e "MALLOC_ARENA_MAX"}

As a result, any value of glibc.malloc.arena_max is ignored by
TUNABLE_SET_VAL_IF_VALID_RANGE macro
  __type min = (__cur)->type.min;                        <- initialized to 1
  __type max = (__cur)->type.max;                        <- initialized to 0!
  if (min == max)                                        <- false
    {
      min = __default_min;
      max = __default_max;
    }
  if ((__type) (__val) >= min && (__type) (val) <= max)  <- false
    {
      (__cur)->val.numval = val;
      (__cur)->initialized = true;
    }

Assigning correct min/max values at a build time fixes a problem.
Plus, a bit of optimization: Setting of default min/max values for the given
type at a run time might be eliminated.
---
 ChangeLog                |  6 ++++++
 elf/dl-tunables.c        | 15 ++++-----------
 scripts/gen-tunables.awk | 12 ++++++++++--
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5944df0..beafe79 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2017-07-19  Alexey Makhalov  <amakhalov@vmware.com>
+
+	* elf/dl-tunables.c (do_tunable_update_val): Range checking fix.
+	* scripts/gen-tunables.awk: Set unspecified minval and/or maxval values
+	to correct default value for given type.
+
 2017-07-18  Szabolcs Nagy  <szabolcs.nagy@arm.com>
 
 	* sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h (_dl_procinfo): Revert.
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 44c160c..8e02a95 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -86,18 +86,11 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
   return NULL;
 }
 
-#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type, __default_min, \
-				       __default_max)			      \
+#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type)		      \
 ({									      \
   __type min = (__cur)->type.min;					      \
   __type max = (__cur)->type.max;					      \
 									      \
-  if (min == max)							      \
-    {									      \
-      min = __default_min;						      \
-      max = __default_max;						      \
-    }									      \
-									      \
   if ((__type) (__val) >= min && (__type) (val) <= max)			      \
     {									      \
       (__cur)->val.numval = val;					      \
@@ -117,17 +110,17 @@ do_tunable_update_val (tunable_t *cur, const void *valp)
     {
     case TUNABLE_TYPE_INT_32:
 	{
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t, INT32_MIN, INT32_MAX);
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t);
 	  break;
 	}
     case TUNABLE_TYPE_UINT_64:
 	{
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, UINT64_MAX);
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t);
 	  break;
 	}
     case TUNABLE_TYPE_SIZE_T:
 	{
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, SIZE_MAX);
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t);
 	  break;
 	}
     case TUNABLE_TYPE_STRING:
diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index ccdd0c6..6221990 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -1,6 +1,14 @@
 # Generate dl-tunable-list.h from dl-tunables.list
 
 BEGIN {
+  min_of["STRING"]="0"
+  max_of["STRING"]="0"
+  min_of["INT_32"]="INT32_MIN"
+  max_of["INT_32"]="INT32_MAX"
+  min_of["UINT_64"]="0"
+  max_of["UINT_64"]="UINT64_MAX"
+  min_of["SIZE_T"]="0"
+  max_of["SIZE_T"]="SIZE_MAX"
   tunable=""
   ns=""
   top_ns=""
@@ -43,10 +51,10 @@ $1 == "}" {
       types[top_ns,ns,tunable] = "STRING"
     }
     if (!minvals[top_ns,ns,tunable]) {
-      minvals[top_ns,ns,tunable] = "0"
+      minvals[top_ns,ns,tunable] = min_of[types[top_ns,ns,tunable]]
     }
     if (!maxvals[top_ns,ns,tunable]) {
-      maxvals[top_ns,ns,tunable] = "0"
+      maxvals[top_ns,ns,tunable] = max_of[types[top_ns,ns,tunable]]
     }
     if (!env_alias[top_ns,ns,tunable]) {
       env_alias[top_ns,ns,tunable] = "NULL"
-- 
2.9.3

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix range check in do_tunable_update_val
  2017-07-19 21:34 [PATCH] Fix range check in do_tunable_update_val Alexey Makhalov
@ 2017-07-20  1:43 ` Carlos O'Donell
  0 siblings, 0 replies; 6+ messages in thread
From: Carlos O'Donell @ 2017-07-20  1:43 UTC (permalink / raw)
  To: Alexey Makhalov, libc-alpha

On 07/19/2017 05:34 PM, Alexey Makhalov wrote:
> Current implementation of tunables does not set arena_max and arena_test
> values. Any value provided by glibc.malloc.arena_max and
> glibc.malloc.arena_test parameters is ignored.
> 
> These tunables have minval value set to 1 (see elf/dl-tunables.list file)
> and undefined maxval value. In that case default value (which is 0. see
> scripts/gen-tunables.awk) is being used to set maxval.

Alexey,

Thank you very much for continuing your contributions!

Your patch has ~10-14 lines of legally significant changes.

Given that VMWare has already contributed ~6 legally significant lines of
code with the pthread_create swbz#20116 changes that we worked on together,
this means we need to continue our discussions about copyright assignment
before we can continue to accept any further patches.

As a reminder, please review the contribution checklist:
https://sourceware.org/glibc/wiki/Contribution%20checklist

-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix range check in do_tunable_update_val
  2017-09-26 20:07   ` Carlos O'Donell
@ 2017-09-26 21:17     ` Siddhesh Poyarekar
  0 siblings, 0 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2017-09-26 21:17 UTC (permalink / raw)
  To: Carlos O'Donell, Alexey Makhalov; +Cc: libc-alpha

On Wednesday 27 September 2017 01:37 AM, Carlos O'Donell wrote:
> Both Alexey and VMWare for Alexey have assignments in place.
> 
> We can accept any patches from Alexey.

Thanks, pushed.

Siddhesh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix range check in do_tunable_update_val
  2017-09-26 18:54 ` Siddhesh Poyarekar
@ 2017-09-26 20:07   ` Carlos O'Donell
  2017-09-26 21:17     ` Siddhesh Poyarekar
  0 siblings, 1 reply; 6+ messages in thread
From: Carlos O'Donell @ 2017-09-26 20:07 UTC (permalink / raw)
  To: Siddhesh Poyarekar, Alexey Makhalov; +Cc: libc-alpha

On 09/26/2017 12:54 PM, Siddhesh Poyarekar wrote:
> On Wednesday 27 September 2017 12:08 AM, Alexey Makhalov wrote:
>> Current implementation of tunables does not set arena_max and arena_test
>> values. Any value provided by glibc.malloc.arena_max and
>> glibc.malloc.arena_test parameters is ignored.
>>
>> These tunables have minval value set to 1 (see elf/dl-tunables.list file)
>> and undefined maxval value. In that case default value (which is 0. see
>> scripts/gen-tunables.awk) is being used to set maxval.
>>
>> For instance, generated tunable_list[] entry for arena_max is:
>> (gdb) p *cur
>> $1 = {name = 0x7ffff7df6217 "glibc.malloc.arena_max",
>>  type = {type_code = TUNABLE_TYPE_SIZE_T, min = 1, max = 0},
>>   val = {numval = 0, strval = 0x0}, initialized = false,
>>    security_level = TUNABLE_SECLEVEL_SXID_IGNORE,
>>     env_alias = 0x7ffff7df622e "MALLOC_ARENA_MAX"}
>>
>> As a result, any value of glibc.malloc.arena_max is ignored by
>> TUNABLE_SET_VAL_IF_VALID_RANGE macro
>>   __type min = (__cur)->type.min;                    <- initialized to 1
>>   __type max = (__cur)->type.max;                    <- initialized to 0!
>>   if (min == max)                                    <- false
>>     {
>>       min = __default_min;
>>       max = __default_max;
>>     }
>>   if ((__type) (__val) >= min && (__type) (val) <= max)  <- false
>>     {
>>       (__cur)->val.numval = val;
>>       (__cur)->initialized = true;
>>     }
>>
>> Assigning correct min/max values at a build time fixes a problem.
>> Plus, a bit of optimization: Setting of default min/max values for the
>> given type at a run time might be eliminated.
> 
> Thanks for the patch.  Can you please clarify the status of your
> copyright assignment (and Carlos verify it please)?  I don't have access
> to the copyright assignment list, so once that question is cleared, I'll
> test and merge this patch.

Both Alexey and VMWare for Alexey have assignments in place.

We can accept any patches from Alexey.

Cheers,
Carlos.



-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix range check in do_tunable_update_val
  2017-09-26 18:38 Alexey Makhalov
@ 2017-09-26 18:54 ` Siddhesh Poyarekar
  2017-09-26 20:07   ` Carlos O'Donell
  0 siblings, 1 reply; 6+ messages in thread
From: Siddhesh Poyarekar @ 2017-09-26 18:54 UTC (permalink / raw)
  To: Alexey Makhalov, carlos; +Cc: libc-alpha

On Wednesday 27 September 2017 12:08 AM, Alexey Makhalov wrote:
> Current implementation of tunables does not set arena_max and arena_test
> values. Any value provided by glibc.malloc.arena_max and
> glibc.malloc.arena_test parameters is ignored.
> 
> These tunables have minval value set to 1 (see elf/dl-tunables.list file)
> and undefined maxval value. In that case default value (which is 0. see
> scripts/gen-tunables.awk) is being used to set maxval.
> 
> For instance, generated tunable_list[] entry for arena_max is:
> (gdb) p *cur
> $1 = {name = 0x7ffff7df6217 "glibc.malloc.arena_max",
>  type = {type_code = TUNABLE_TYPE_SIZE_T, min = 1, max = 0},
>   val = {numval = 0, strval = 0x0}, initialized = false,
>    security_level = TUNABLE_SECLEVEL_SXID_IGNORE,
>     env_alias = 0x7ffff7df622e "MALLOC_ARENA_MAX"}
> 
> As a result, any value of glibc.malloc.arena_max is ignored by
> TUNABLE_SET_VAL_IF_VALID_RANGE macro
>   __type min = (__cur)->type.min;                    <- initialized to 1
>   __type max = (__cur)->type.max;                    <- initialized to 0!
>   if (min == max)                                    <- false
>     {
>       min = __default_min;
>       max = __default_max;
>     }
>   if ((__type) (__val) >= min && (__type) (val) <= max)  <- false
>     {
>       (__cur)->val.numval = val;
>       (__cur)->initialized = true;
>     }
> 
> Assigning correct min/max values at a build time fixes a problem.
> Plus, a bit of optimization: Setting of default min/max values for the
> given type at a run time might be eliminated.

Thanks for the patch.  Can you please clarify the status of your
copyright assignment (and Carlos verify it please)?  I don't have access
to the copyright assignment list, so once that question is cleared, I'll
test and merge this patch.

Siddhesh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] Fix range check in do_tunable_update_val
@ 2017-09-26 18:38 Alexey Makhalov
  2017-09-26 18:54 ` Siddhesh Poyarekar
  0 siblings, 1 reply; 6+ messages in thread
From: Alexey Makhalov @ 2017-09-26 18:38 UTC (permalink / raw)
  To: carlos; +Cc: libc-alpha, Alexey Makhalov

Current implementation of tunables does not set arena_max and arena_test
values. Any value provided by glibc.malloc.arena_max and
glibc.malloc.arena_test parameters is ignored.

These tunables have minval value set to 1 (see elf/dl-tunables.list file)
and undefined maxval value. In that case default value (which is 0. see
scripts/gen-tunables.awk) is being used to set maxval.

For instance, generated tunable_list[] entry for arena_max is:
(gdb) p *cur
$1 = {name = 0x7ffff7df6217 "glibc.malloc.arena_max",
 type = {type_code = TUNABLE_TYPE_SIZE_T, min = 1, max = 0},
  val = {numval = 0, strval = 0x0}, initialized = false,
   security_level = TUNABLE_SECLEVEL_SXID_IGNORE,
    env_alias = 0x7ffff7df622e "MALLOC_ARENA_MAX"}

As a result, any value of glibc.malloc.arena_max is ignored by
TUNABLE_SET_VAL_IF_VALID_RANGE macro
  __type min = (__cur)->type.min;                    <- initialized to 1
  __type max = (__cur)->type.max;                    <- initialized to 0!
  if (min == max)                                    <- false
    {
      min = __default_min;
      max = __default_max;
    }
  if ((__type) (__val) >= min && (__type) (val) <= max)  <- false
    {
      (__cur)->val.numval = val;
      (__cur)->initialized = true;
    }

Assigning correct min/max values at a build time fixes a problem.
Plus, a bit of optimization: Setting of default min/max values for the
given type at a run time might be eliminated.

        * elf/dl-tunables.c (do_tunable_update_val): Range checking fix.
        * scripts/gen-tunables.awk: Set unspecified minval and/or maxval
	values to correct default value for given type.
---
 elf/dl-tunables.c        | 15 ++++-----------
 scripts/gen-tunables.awk | 12 ++++++++++--
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index b964a09..cc2c637 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -89,18 +89,11 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
   return NULL;
 }
 
-#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type, __default_min, \
-				       __default_max)			      \
+#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type)		      \
 ({									      \
   __type min = (__cur)->type.min;					      \
   __type max = (__cur)->type.max;					      \
 									      \
-  if (min == max)							      \
-    {									      \
-      min = __default_min;						      \
-      max = __default_max;						      \
-    }									      \
-									      \
   if ((__type) (__val) >= min && (__type) (val) <= max)			      \
     {									      \
       (__cur)->val.numval = val;					      \
@@ -120,17 +113,17 @@ do_tunable_update_val (tunable_t *cur, const void *valp)
     {
     case TUNABLE_TYPE_INT_32:
 	{
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t, INT32_MIN, INT32_MAX);
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t);
 	  break;
 	}
     case TUNABLE_TYPE_UINT_64:
 	{
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, UINT64_MAX);
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t);
 	  break;
 	}
     case TUNABLE_TYPE_SIZE_T:
 	{
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, SIZE_MAX);
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t);
 	  break;
 	}
     case TUNABLE_TYPE_STRING:
diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index ccdd0c6..6221990 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -1,6 +1,14 @@
 # Generate dl-tunable-list.h from dl-tunables.list
 
 BEGIN {
+  min_of["STRING"]="0"
+  max_of["STRING"]="0"
+  min_of["INT_32"]="INT32_MIN"
+  max_of["INT_32"]="INT32_MAX"
+  min_of["UINT_64"]="0"
+  max_of["UINT_64"]="UINT64_MAX"
+  min_of["SIZE_T"]="0"
+  max_of["SIZE_T"]="SIZE_MAX"
   tunable=""
   ns=""
   top_ns=""
@@ -43,10 +51,10 @@ $1 == "}" {
       types[top_ns,ns,tunable] = "STRING"
     }
     if (!minvals[top_ns,ns,tunable]) {
-      minvals[top_ns,ns,tunable] = "0"
+      minvals[top_ns,ns,tunable] = min_of[types[top_ns,ns,tunable]]
     }
     if (!maxvals[top_ns,ns,tunable]) {
-      maxvals[top_ns,ns,tunable] = "0"
+      maxvals[top_ns,ns,tunable] = max_of[types[top_ns,ns,tunable]]
     }
     if (!env_alias[top_ns,ns,tunable]) {
       env_alias[top_ns,ns,tunable] = "NULL"
-- 
2.9.3

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-09-26 21:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-19 21:34 [PATCH] Fix range check in do_tunable_update_val Alexey Makhalov
2017-07-20  1:43 ` Carlos O'Donell
2017-09-26 18:38 Alexey Makhalov
2017-09-26 18:54 ` Siddhesh Poyarekar
2017-09-26 20:07   ` Carlos O'Donell
2017-09-26 21:17     ` Siddhesh Poyarekar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).