[PATCH] tunables: Fix comparison of tunable values

[PATCH] tunables: Fix comparison of tunable values

[Siddhesh Poyarekar]

The simplification of tunable_set interfaces took care of
signed/unsigned conversions while setting values, but comparison with
bounds ended up being incorrect; comparing TUNABLE_SIZE_T values for
example will fail because SIZE_MAX is seen as -1.

Add comparison helpers that take tunable types into account and use
them to do comparison instead.
---
 elf/dl-tunable-types.h | 17 +++++++++++++++++
 elf/dl-tunables.c      | 25 ++++++++++++++-----------
 elf/dl-tunables.h      | 18 ++++++++++++++++++
 3 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
index 626ca334be..39bf738d93 100644
--- a/elf/dl-tunable-types.h
+++ b/elf/dl-tunable-types.h
@@ -81,4 +81,21 @@ struct _tunable
 
 typedef struct _tunable tunable_t;
 
+static __always_inline bool
+unsigned_tunable_type (tunable_type_code_t t)
+{
+  switch (t)
+    {
+    case TUNABLE_TYPE_INT_32:
+      return false;
+    case TUNABLE_TYPE_UINT_64:
+    case TUNABLE_TYPE_SIZE_T:
+      return true;
+    case TUNABLE_TYPE_STRING:
+    default:
+      break;
+    }
+  __builtin_unreachable ();
+}
+
 #endif
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 1aedb9bd36..8009e54ee5 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -107,32 +107,35 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
       return;
     }
 
+  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;
 
   /* We allow only increasingly restrictive bounds.  */
-  if (min < cur->type.min)
+  if (tunable_val_lt (min, cur->type.min, unsigned_cmp))
     min = cur->type.min;
 
-  if (max > cur->type.max)
+  if (tunable_val_gt (max, cur->type.max, unsigned_cmp))
     max = cur->type.max;
 
   /* Skip both bounds if they're inconsistent.  */
-  if (min > max)
+  if (tunable_val_gt (min, max, unsigned_cmp))
     {
       min = cur->type.min;
       max = cur->type.max;
     }
 
-  /* Write everything out if the value and the bounds are valid.  */
-  if (min <= val && val <= max)
-    {
-      cur->val.numval = val;
-      cur->type.min = min;
-      cur->type.max = max;
-      cur->initialized = true;
-    }
+  /* Bail out if the bounds are not valid.  */
+  if (tunable_val_lt (val, min, unsigned_cmp)
+      || tunable_val_lt (max, val, unsigned_cmp))
+    return;
+
+  cur->val.numval = val;
+  cur->type.min = min;
+  cur->type.max = max;
+  cur->initialized = true;
 }
 
 /* Validate range of the input value and initialize the tunable CUR if it looks
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
index ba7ae6b52e..3880e4aab6 100644
--- a/elf/dl-tunables.h
+++ b/elf/dl-tunables.h
@@ -115,6 +115,24 @@ rtld_hidden_proto (__tunable_set_val)
 /* The default value for TUNABLES_FRONTEND.  */
 # define TUNABLES_FRONTEND_yes TUNABLES_FRONTEND_valstring
 
+static __always_inline bool
+tunable_val_lt (tunable_num_t lhs, tunable_num_t rhs, bool unsigned_cmp)
+{
+  if (unsigned_cmp)
+    return (uintmax_t) lhs < (uintmax_t) rhs;
+  else
+    return lhs < rhs;
+}
+
+static __always_inline bool
+tunable_val_gt (tunable_num_t lhs, tunable_num_t rhs, bool unsigned_cmp)
+{
+  if (unsigned_cmp)
+    return (uintmax_t) lhs > (uintmax_t) rhs;
+  else
+    return lhs > rhs;
+}
+
 /* Compare two name strings, bounded by the name hardcoded in glibc.  */
 static __always_inline bool
 tunable_is_name (const char *orig, const char *envname)
-- 
2.29.2

[PING][PATCH] tunables: Fix comparison of tunable values

[Siddhesh Poyarekar]

On 3/16/21 7:04 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> The simplification of tunable_set interfaces took care of
> signed/unsigned conversions while setting values, but comparison with
> bounds ended up being incorrect; comparing TUNABLE_SIZE_T values for
> example will fail because SIZE_MAX is seen as -1.
> 
> Add comparison helpers that take tunable types into account and use
> them to do comparison instead.
> ---
>   elf/dl-tunable-types.h | 17 +++++++++++++++++
>   elf/dl-tunables.c      | 25 ++++++++++++++-----------
>   elf/dl-tunables.h      | 18 ++++++++++++++++++
>   3 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index 626ca334be..39bf738d93 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -81,4 +81,21 @@ struct _tunable
>   
>   typedef struct _tunable tunable_t;
>   
> +static __always_inline bool
> +unsigned_tunable_type (tunable_type_code_t t)
> +{
> +  switch (t)
> +    {
> +    case TUNABLE_TYPE_INT_32:
> +      return false;
> +    case TUNABLE_TYPE_UINT_64:
> +    case TUNABLE_TYPE_SIZE_T:
> +      return true;
> +    case TUNABLE_TYPE_STRING:
> +    default:
> +      break;
> +    }
> +  __builtin_unreachable ();
> +}
> +
>   #endif
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 1aedb9bd36..8009e54ee5 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -107,32 +107,35 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
>         return;
>       }
>   
> +  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;
>   
>     /* We allow only increasingly restrictive bounds.  */
> -  if (min < cur->type.min)
> +  if (tunable_val_lt (min, cur->type.min, unsigned_cmp))
>       min = cur->type.min;
>   
> -  if (max > cur->type.max)
> +  if (tunable_val_gt (max, cur->type.max, unsigned_cmp))
>       max = cur->type.max;
>   
>     /* Skip both bounds if they're inconsistent.  */
> -  if (min > max)
> +  if (tunable_val_gt (min, max, unsigned_cmp))
>       {
>         min = cur->type.min;
>         max = cur->type.max;
>       }
>   
> -  /* Write everything out if the value and the bounds are valid.  */
> -  if (min <= val && val <= max)
> -    {
> -      cur->val.numval = val;
> -      cur->type.min = min;
> -      cur->type.max = max;
> -      cur->initialized = true;
> -    }
> +  /* Bail out if the bounds are not valid.  */
> +  if (tunable_val_lt (val, min, unsigned_cmp)
> +      || tunable_val_lt (max, val, unsigned_cmp))
> +    return;
> +
> +  cur->val.numval = val;
> +  cur->type.min = min;
> +  cur->type.max = max;
> +  cur->initialized = true;
>   }
>   
>   /* Validate range of the input value and initialize the tunable CUR if it looks
> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> index ba7ae6b52e..3880e4aab6 100644
> --- a/elf/dl-tunables.h
> +++ b/elf/dl-tunables.h
> @@ -115,6 +115,24 @@ rtld_hidden_proto (__tunable_set_val)
>   /* The default value for TUNABLES_FRONTEND.  */
>   # define TUNABLES_FRONTEND_yes TUNABLES_FRONTEND_valstring
>   
> +static __always_inline bool
> +tunable_val_lt (tunable_num_t lhs, tunable_num_t rhs, bool unsigned_cmp)
> +{
> +  if (unsigned_cmp)
> +    return (uintmax_t) lhs < (uintmax_t) rhs;
> +  else
> +    return lhs < rhs;
> +}
> +
> +static __always_inline bool
> +tunable_val_gt (tunable_num_t lhs, tunable_num_t rhs, bool unsigned_cmp)
> +{
> +  if (unsigned_cmp)
> +    return (uintmax_t) lhs > (uintmax_t) rhs;
> +  else
> +    return lhs > rhs;
> +}
> +
>   /* Compare two name strings, bounded by the name hardcoded in glibc.  */
>   static __always_inline bool
>   tunable_is_name (const char *orig, const char *envname)
> 

Re: [PATCH] tunables: Fix comparison of tunable values

[Carlos O'Donell]

On 3/16/21 9:34 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> The simplification of tunable_set interfaces took care of
> signed/unsigned conversions while setting values, but comparison with
> bounds ended up being incorrect; comparing TUNABLE_SIZE_T values for
> example will fail because SIZE_MAX is seen as -1.
> 
> Add comparison helpers that take tunable types into account and use
> them to do comparison instead.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/dl-tunable-types.h | 17 +++++++++++++++++
>  elf/dl-tunables.c      | 25 ++++++++++++++-----------
>  elf/dl-tunables.h      | 18 ++++++++++++++++++
>  3 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index 626ca334be..39bf738d93 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -81,4 +81,21 @@ struct _tunable
>  
>  typedef struct _tunable tunable_t;
>  
> +static __always_inline bool
> +unsigned_tunable_type (tunable_type_code_t t)
> +{
> +  switch (t)
> +    {
> +    case TUNABLE_TYPE_INT_32:
> +      return false;
> +    case TUNABLE_TYPE_UINT_64:
> +    case TUNABLE_TYPE_SIZE_T:
> +      return true;
> +    case TUNABLE_TYPE_STRING:
> +    default:
> +      break;
> +    }
> +  __builtin_unreachable ();
> +}
> +

OK.

>  #endif
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 1aedb9bd36..8009e54ee5 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -107,32 +107,35 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
>        return;
>      }
>  
> +  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;
>  
>    /* We allow only increasingly restrictive bounds.  */
> -  if (min < cur->type.min)
> +  if (tunable_val_lt (min, cur->type.min, unsigned_cmp))
>      min = cur->type.min;
>  
> -  if (max > cur->type.max)
> +  if (tunable_val_gt (max, cur->type.max, unsigned_cmp))
>      max = cur->type.max;
>  
>    /* Skip both bounds if they're inconsistent.  */
> -  if (min > max)
> +  if (tunable_val_gt (min, max, unsigned_cmp))

OK.

>      {
>        min = cur->type.min;
>        max = cur->type.max;
>      }
>  
> -  /* Write everything out if the value and the bounds are valid.  */
> -  if (min <= val && val <= max)
> -    {
> -      cur->val.numval = val;
> -      cur->type.min = min;
> -      cur->type.max = max;
> -      cur->initialized = true;
> -    }
> +  /* Bail out if the bounds are not valid.  */
> +  if (tunable_val_lt (val, min, unsigned_cmp)
> +      || tunable_val_lt (max, val, unsigned_cmp))
> +    return;
> +
> +  cur->val.numval = val;
> +  cur->type.min = min;
> +  cur->type.max = max;
> +  cur->initialized = true;
>  }
>  
>  /* Validate range of the input value and initialize the tunable CUR if it looks
> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> index ba7ae6b52e..3880e4aab6 100644
> --- a/elf/dl-tunables.h
> +++ b/elf/dl-tunables.h
> @@ -115,6 +115,24 @@ rtld_hidden_proto (__tunable_set_val)
>  /* The default value for TUNABLES_FRONTEND.  */
>  # define TUNABLES_FRONTEND_yes TUNABLES_FRONTEND_valstring
>  
> +static __always_inline bool
> +tunable_val_lt (tunable_num_t lhs, tunable_num_t rhs, bool unsigned_cmp)
> +{
> +  if (unsigned_cmp)
> +    return (uintmax_t) lhs < (uintmax_t) rhs;
> +  else
> +    return lhs < rhs;
> +}
> +
> +static __always_inline bool
> +tunable_val_gt (tunable_num_t lhs, tunable_num_t rhs, bool unsigned_cmp)
> +{
> +  if (unsigned_cmp)
> +    return (uintmax_t) lhs > (uintmax_t) rhs;
> +  else
> +    return lhs > rhs;
> +}
> +
>  /* Compare two name strings, bounded by the name hardcoded in glibc.  */
>  static __always_inline bool
>  tunable_is_name (const char *orig, const char *envname)
> 


-- 
Cheers,
Carlos.