[1/3] Fix casts when setting tunable range

Cast tunable min and max to target type before comparison so that we
don't mix comparison of signed and unsigned types.
---
 elf/dl-tunables.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

On 03/02/2021 14:34, Siddhesh Poyarekar via Libc-alpha wrote:
> Cast tunable min and max to target type before comparison so that we
> don't mix comparison of signed and unsigned types.
> ---
>  elf/dl-tunables.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index b1a50b8469..488be4bbf1 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -107,6 +107,8 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
>  
>  #define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __minp, __maxp, __type)	      \
>  ({									      \
> +  __type curmin = (__cur)->type.min;				      \
> +  __type curmax = (__cur)->type.max;				      \
>    if (__minp != NULL)							      \
>      {									      \
>        /* MIN is specified.  */						      \
> @@ -115,15 +117,13 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
>  	{								      \
>  	   /* Both MIN and MAX are specified.  */			      \
>  	    __type max = *((__type *) __maxp);				      \
> -	  if (max >= min						      \
> -	      && max <= (__cur)->type.max				      \
> -	      && min >= (__cur)->type.min)				      \
> +	  if (max >= min && max <= curmax && min >= curmin)		      \
>  	    {								      \
>  	      (__cur)->type.min = min;					      \
>  	      (__cur)->type.max = max;					      \
>  	    }								      \
>  	}								      \
> -      else if (min > (__cur)->type.min && min <= (__cur)->type.max)	      \
> +      else if (min > curmin && min <= curmax)				      \
>  	{								      \
>  	  /* Only MIN is specified.  */					      \
>  	  (__cur)->type.min = min;					      \
> @@ -133,7 +133,7 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
>      {									      \
>        /* Only MAX is specified.  */					      \
>        __type max = *((__type *) __maxp);				      \
> -      if (max < (__cur)->type.max && max >= (__cur)->type.min)		      \
> +      if (max < curmax && max >= curmin)				      \
>  	(__cur)->type.max = max;					      \
>      }									      \
>  })
> 

Wouldn't be better to just get rid of this macro mess (which has cause
some issue on the internal syscall mechanism where macro arguments were
not correctly evaluated) and use proper functions instead?

Something like (not sure about the function generation macros, open coded
them might be clear since I don't foresee we adding more different type):

---

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index b1a50b8469..fa9bccfb29 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -93,50 +93,52 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
   return NULL;
 }
 
-#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type)		      \
-({									      \
-  __type min = (__cur)->type.min;					      \
-  __type max = (__cur)->type.max;					      \
-									      \
-  if ((__type) (__val) >= min && (__type) (__val) <= max)		      \
-    {									      \
-      (__cur)->val.numval = (__val);					      \
-      (__cur)->initialized = true;					      \
-    }									      \
-})
-
-#define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __minp, __maxp, __type)	      \
-({									      \
-  if (__minp != NULL)							      \
-    {									      \
-      /* MIN is specified.  */						      \
-      __type min = *((__type *) __minp);				      \
-      if (__maxp != NULL)						      \
-	{								      \
-	   /* Both MIN and MAX are specified.  */			      \
-	    __type max = *((__type *) __maxp);				      \
-	  if (max >= min						      \
-	      && max <= (__cur)->type.max				      \
-	      && min >= (__cur)->type.min)				      \
-	    {								      \
-	      (__cur)->type.min = min;					      \
-	      (__cur)->type.max = max;					      \
-	    }								      \
-	}								      \
-      else if (min > (__cur)->type.min && min <= (__cur)->type.max)	      \
-	{								      \
-	  /* Only MIN is specified.  */					      \
-	  (__cur)->type.min = min;					      \
-	}								      \
-    }									      \
-  else if (__maxp != NULL)						      \
-    {									      \
-      /* Only MAX is specified.  */					      \
-      __type max = *((__type *) __maxp);				      \
-      if (max < (__cur)->type.max && max >= (__cur)->type.min)		      \
-	(__cur)->type.max = max;					      \
-    }									      \
-})
+#define DEFINE_TUNABLE_SET_VAL_IF_VALID_RANGE(__type)			\
+static void								\
+do_tunable_set_val_if_valid_range_##__type(tunable_t *cur, __type val)	\
+{									\
+  __type min = cur->type.min;						\
+  __type max = cur->type.max;						\
+  if (val >= min && val <= max)						\
+    {									\
+      cur->val.numval = val;						\
+      cur->initialized = true;						\
+    }									\
+}
+DEFINE_TUNABLE_SET_VAL_IF_VALID_RANGE (int64_t)
+DEFINE_TUNABLE_SET_VAL_IF_VALID_RANGE (uint64_t)
+
+#define DEFINE_TUNABLE_SET_BOUNDS_IF_VALID(__type) 			\
+static void								\
+do_tunable_update_val_##__type (tunable_t *cur, const __type *minp,	\
+				const __type *maxp) 			\
+{									\
+  if (minp != NULL)							\
+    {									\
+      __type min = *minp;						\
+      if (maxp != NULL)							\
+	{								\
+	  __type max = *maxp;						\
+	  if (max >= min						\
+	      && max <= cur->type.max					\
+	      && min >= cur->type.min)					\
+	    {								\
+	      cur->type.min = min;					\
+	      cur->type.max = max;					\
+	    }								\
+	}								\
+      else if (min > cur->type.min && min <= cur->type.max)		\
+	cur->type.min = min;						\
+    }									\
+  else if (maxp != NULL)						\
+    {									\
+      __type max = *maxp;						\
+      if (max < cur->type.max && max >= cur->type.min)			\
+	cur->type.max = max;						\
+    }									\
+}
+DEFINE_TUNABLE_SET_BOUNDS_IF_VALID (int64_t)
+DEFINE_TUNABLE_SET_BOUNDS_IF_VALID (uint64_t)
 
 static void
 do_tunable_update_val (tunable_t *cur, const void *valp,
@@ -150,28 +152,17 @@ do_tunable_update_val (tunable_t *cur, const void *valp,
   switch (cur->type.type_code)
     {
     case TUNABLE_TYPE_INT_32:
-	{
-	  TUNABLE_SET_BOUNDS_IF_VALID (cur, minp, maxp, int64_t);
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t);
-	  break;
-	}
+      do_tunable_update_val_int64_t (cur, minp, maxp);
+      do_tunable_set_val_if_valid_range_int64_t (cur, val);
+      break;
     case TUNABLE_TYPE_UINT_64:
-	{
-	  TUNABLE_SET_BOUNDS_IF_VALID (cur, minp, maxp, uint64_t);
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t);
-	  break;
-	}
     case TUNABLE_TYPE_SIZE_T:
-	{
-	  TUNABLE_SET_BOUNDS_IF_VALID (cur, minp, maxp, uint64_t);
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t);
-	  break;
-	}
+      do_tunable_update_val_uint64_t (cur, minp, maxp);
+      do_tunable_set_val_if_valid_range_uint64_t (cur, val);
+      break;
     case TUNABLE_TYPE_STRING:
-	{
-	  cur->val.strval = valp;
-	  break;
-	}
+      cur->val.strval = valp;
+      break;
     default:
       __builtin_unreachable ();
     }

On 2/4/21 7:14 PM, Adhemerval Zanella wrote:
> Wouldn't be better to just get rid of this macro mess (which has cause
> some issue on the internal syscall mechanism where macro arguments were
> not correctly evaluated) and use proper functions instead?
> 
> Something like (not sure about the function generation macros, open coded
> them might be clear since I don't foresee we adding more different type):

Yeah it's actually just signed vs unsigned, I can definitely make this 
macro-less.  I'll post an update for this patch tomorrow.

Thanks,
Siddhesh