diff mbox series

[committed] openmp: Fix up strtoul and strtoull uses in libgomp

Message ID 20211015144633.GF304296@tucnak
State Committed
Headers
Series [committed] openmp: Fix up strtoul and strtoull uses in libgomp |

Commit Message

Jakub Jelinek Oct. 15, 2021, 2:46 p.m. UTC 
  Hi!

Yesterday when working on numa_domains, I've noticed because of a bug
in my patch a hang on a large NUMA machine.  I've fixed the bug, but
also discovered that the hang was a result of making wrong assumptions
about strtoul/strtoull.  All the uses were for portability setting
errno = 0 before the calls and treating non-zero errno after the call
as invalid input, but for the case where there are no valid digits at
all strtoul may set errno to EINVAL, but doesn't have to and with
glibc doesn't do that.  So, this patch goes through all the strtoul calls
and next to errno != 0 checks adds also endptr == startptr check.
Haven't done it in places where we immediately reject strtoul returning 0
the same as we reject errno != 0, because strtoul must return 0 in the
case where it sets endptr to the start pointer.  In some spots the code
was using errno = 0; x = strtoul (p, &p, 10); if (errno) { /*invalid*/ }
and those spots had to be changed to
errno = 0; x = strtoul (p, &end, 10); if (errno || end == p) { /*invalid*/ }
p = end;

Regtested on x86_64-linux and i686-linux, committed to trunk.

2021-10-15  Jakub Jelinek  <jakub@redhat.com>

	* env.c (parse_schedule): For strtoul or strtoull calls which don't
	clearly reject return value 0 as invalid handle the case where end
	pointer is the same as first argument as invalid.
	(parse_unsigned_long_1): Likewise.
	(parse_one_place): Likewise.
	(parse_places_var): Likewise.
	(parse_stacksize): Likewise.
	(parse_spincount): Likewise.
	(parse_affinity): Likewise.
	(parse_gomp_openacc_dim): Likewise.  Avoid strict aliasing violation.
	Make code valid C89.
	* config/linux/affinity.c (gomp_affinity_find_last_cache_level):
	For strtoul calls which don't clearly reject return value 0 as
	invalid handle the case where end pointer is the same as first
	argument as invalid.
	(gomp_affinity_init_level_1): Likewise.
	(gomp_affinity_init_numa_domains): Likewise.
	* config/rtems/proc.c (parse_thread_pools): Likewise.


	Jakub

Comments

Thomas Schwinge Nov. 9, 2021, 4:06 p.m. UTC | #1 
Hi!

On 2021-10-15T16:46:33+0200, Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> also discovered that the hang was a result of making wrong assumptions
> about strtoul/strtoull.

(Also 'strtol'.)  ;-)

> All the uses were for portability setting
> errno = 0 before the calls and treating non-zero errno after the call
> as invalid input, but for the case where there are no valid digits at
> all strtoul may set errno to EINVAL, but doesn't have to and with
> glibc doesn't do that.  So, this patch goes through all the strtoul calls
> and next to errno != 0 checks adds also endptr == startptr check.
> Haven't done it in places where we immediately reject strtoul returning 0
> the same as we reject errno != 0, because strtoul must return 0 in the
> case where it sets endptr to the start pointer.  In some spots the code
> was using errno = 0; x = strtoul (p, &p, 10); if (errno) { /*invalid*/ }
> and those spots had to be changed to
> errno = 0; x = strtoul (p, &end, 10); if (errno || end == p) { /*invalid*/ }
> p = end;

ACK.

> Regtested on x86_64-linux and i686-linux, committed to trunk.

Thanks for addressing that one, too -- but evidently not properly tested
the one OpenACC change:

>       (parse_gomp_openacc_dim): Likewise.  Avoid strict aliasing violation.
>       Make code valid C89.

(Why the C89 "re-formatting", by the way?  Surely we're "violating" that
in a lot of other places?)

> --- libgomp/env.c.jj  2021-10-14 22:04:30.594333475 +0200
> +++ libgomp/env.c     2021-10-15 14:07:07.464919497 +0200

> @@ -1202,27 +1207,30 @@ parse_gomp_openacc_dim (void)
>    /* The syntax is the same as for the -fopenacc-dim compilation option.  */
>    const char *var_name = "GOMP_OPENACC_DIM";
>    const char *env_var = getenv (var_name);
> +  const char *pos = env_var;
> +  int i;
> +
>    if (!env_var)
>      return;
>
> -  const char *pos = env_var;
> -  int i;
>    for (i = 0; *pos && i != GOMP_DIM_MAX; i++)
>      {
> +      char *eptr;
> +      long val;
> +
>        if (i && *pos++ != ':')
>       break;
>
>        if (*pos == ':')
>       continue;
>
> -      const char *eptr;
>        errno = 0;
> -      long val = strtol (pos, (char **)&eptr, 10);
> -      if (errno || val < 0 || (unsigned)val != val)
> +      val = strtol (pos, &eptr, 10);
> +      if (errno || eptr != pos || val < 0 || (unsigned)val != val)
>       break;

Instead of 'eptr != pos', this needs to be 'eptr == pos', like everywhere
else.

(That there are no diagnostics for malformed 'GOMP_OPENACC_DIM', is a
different topic, of course.)

>
>        goacc_default_dims[i] = (int)val;
> -      pos = eptr;
> +      pos = (const char *) eptr;
>      }
>  }

Pushed to master branch commit 00c9ce13a64e324dabd8dfd236882919a3119479
"Restore 'GOMP_OPENACC_DIM' environment variable parsing", see attached.


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
diff mbox series

Patch

--- libgomp/env.c.jj	2021-10-14 22:04:30.594333475 +0200
+++ libgomp/env.c	2021-10-15 14:07:07.464919497 +0200
@@ -183,7 +183,7 @@  parse_schedule (void)
 
   errno = 0;
   value = strtoul (env, &end, 10);
-  if (errno)
+  if (errno || end == env)
     goto invalid;
 
   while (isspace ((unsigned char) *end))
@@ -232,7 +232,7 @@  parse_unsigned_long_1 (const char *name,
 
   errno = 0;
   value = strtoul (env, &end, 10);
-  if (errno || (long) value <= 0 - allow_zero)
+  if (errno || end == env || (long) value <= 0 - allow_zero)
     goto invalid;
 
   while (isspace ((unsigned char) *end))
@@ -570,6 +570,7 @@  parse_one_place (char **envp, bool *nega
 	  unsigned long this_num, this_len = 1;
 	  long this_stride = 1;
 	  bool this_negate = (*env == '!');
+	  char *end;
 	  if (this_negate)
 	    {
 	      if (gomp_places_list)
@@ -580,9 +581,10 @@  parse_one_place (char **envp, bool *nega
 	    }
 
 	  errno = 0;
-	  this_num = strtoul (env, &env, 10);
-	  if (errno)
+	  this_num = strtoul (env, &end, 10);
+	  if (errno || end == env)
 	    return false;
+	  env = end;
 	  while (isspace ((unsigned char) *env))
 	    ++env;
 	  if (*env == ':')
@@ -602,9 +604,10 @@  parse_one_place (char **envp, bool *nega
 		  while (isspace ((unsigned char) *env))
 		    ++env;
 		  errno = 0;
-		  this_stride = strtol (env, &env, 10);
-		  if (errno)
+		  this_stride = strtol (env, &end, 10);
+		  if (errno || end == env)
 		    return false;
+		  env = end;
 		  while (isspace ((unsigned char) *env))
 		    ++env;
 		}
@@ -636,6 +639,7 @@  parse_one_place (char **envp, bool *nega
     ++env;
   if (*env == ':')
     {
+      char *end;
       ++env;
       while (isspace ((unsigned char) *env))
 	++env;
@@ -651,9 +655,10 @@  parse_one_place (char **envp, bool *nega
 	  while (isspace ((unsigned char) *env))
 	    ++env;
 	  errno = 0;
-	  stride = strtol (env, &env, 10);
-	  if (errno)
+	  stride = strtol (env, &end, 10);
+	  if (errno || end == env)
 	    return false;
+	  env = end;
 	  while (isspace ((unsigned char) *env))
 	    ++env;
 	}
@@ -720,7 +725,7 @@  parse_places_var (const char *name, bool
 
 	  errno = 0;
 	  count = strtoul (env, &end, 10);
-	  if (errno)
+	  if (errno || end == env)
 	    goto invalid;
 	  env = end;
 	  while (isspace ((unsigned char) *env))
@@ -859,7 +864,7 @@  parse_stacksize (const char *name, unsig
 
   errno = 0;
   value = strtoul (env, &end, 10);
-  if (errno)
+  if (errno || end == env)
     goto invalid;
 
   while (isspace ((unsigned char) *end))
@@ -928,7 +933,7 @@  parse_spincount (const char *name, unsig
 
   errno = 0;
   value = strtoull (env, &end, 10);
-  if (errno)
+  if (errno || end == env)
     goto invalid;
 
   while (isspace ((unsigned char) *end))
@@ -1080,7 +1085,7 @@  parse_affinity (bool ignore)
 
 	  errno = 0;
 	  cpu_beg = strtoul (env, &end, 0);
-	  if (errno || cpu_beg >= 65536)
+	  if (errno || end == env || cpu_beg >= 65536)
 	    goto invalid;
 	  cpu_end = cpu_beg;
 	  cpu_stride = 1;
@@ -1090,7 +1095,7 @@  parse_affinity (bool ignore)
 	    {
 	      errno = 0;
 	      cpu_end = strtoul (++env, &end, 0);
-	      if (errno || cpu_end >= 65536 || cpu_end < cpu_beg)
+	      if (errno || end == env || cpu_end >= 65536 || cpu_end < cpu_beg)
 		goto invalid;
 
 	      env = end;
@@ -1202,27 +1207,30 @@  parse_gomp_openacc_dim (void)
   /* The syntax is the same as for the -fopenacc-dim compilation option.  */
   const char *var_name = "GOMP_OPENACC_DIM";
   const char *env_var = getenv (var_name);
+  const char *pos = env_var;
+  int i;
+
   if (!env_var)
     return;
 
-  const char *pos = env_var;
-  int i;
   for (i = 0; *pos && i != GOMP_DIM_MAX; i++)
     {
+      char *eptr;
+      long val;
+
       if (i && *pos++ != ':')
 	break;
 
       if (*pos == ':')
 	continue;
 
-      const char *eptr;
       errno = 0;
-      long val = strtol (pos, (char **)&eptr, 10);
-      if (errno || val < 0 || (unsigned)val != val)
+      val = strtol (pos, &eptr, 10);
+      if (errno || eptr != pos || val < 0 || (unsigned)val != val)
 	break;
 
       goacc_default_dims[i] = (int)val;
-      pos = eptr;
+      pos = (const char *) eptr;
     }
 }
 
--- libgomp/config/linux/affinity.c.jj	2021-10-15 13:20:19.561484351 +0200
+++ libgomp/config/linux/affinity.c	2021-10-15 14:14:14.718752064 +0200
@@ -251,7 +251,7 @@  gomp_affinity_find_last_cache_level (cha
 	  char *p;
 	  errno = 0;
 	  val = strtoul (line, &p, 10);
-	  if (!errno && val >= maxval)
+	  if (!errno && p > line && val >= maxval)
 	    {
 	      ret = l;
 	      maxval = val;
@@ -303,7 +303,7 @@  gomp_affinity_init_level_1 (int level, i
 	  }
 	if (getline (&line, &linelen, f) > 0)
 	  {
-	    char *p = line;
+	    char *p = line, *end;
 	    void *pl = gomp_places_list[gomp_places_list_len];
 	    if (level == this_level)
 	      gomp_affinity_init_place (pl);
@@ -311,16 +311,18 @@  gomp_affinity_init_level_1 (int level, i
 	      {
 		unsigned long first, last;
 		errno = 0;
-		first = strtoul (p, &p, 10);
-		if (errno)
+		first = strtoul (p, &end, 10);
+		if (errno || end == p)
 		  break;
+		p = end;
 		last = first;
 		if (*p == '-')
 		  {
 		    errno = 0;
-		    last = strtoul (p + 1, &p, 10);
-		    if (errno || last < first)
+		    last = strtoul (p + 1, &end, 10);
+		    if (errno || end == p + 1 || last < first)
 		      break;
+		    p = end;
 		  }
 		for (; first <= last; first++)
 		  if (!CPU_ISSET_S (first, gomp_cpuset_size, copy))
@@ -383,18 +385,21 @@  gomp_affinity_init_numa_domains (unsigne
   while (*q && *q != '\n' && gomp_places_list_len < count)
     {
       unsigned long nfirst, nlast;
+      char *end;
 
       errno = 0;
-      nfirst = strtoul (q, &q, 10);
-      if (errno)
+      nfirst = strtoul (q, &end, 10);
+      if (errno || end == q)
 	break;
+      q = end;
       nlast = nfirst;
       if (*q == '-')
 	{
 	  errno = 0;
-	  nlast = strtoul (q + 1, &q, 10);
-	  if (errno || nlast < nfirst)
+	  nlast = strtoul (q + 1, &end, 10);
+	  if (errno || end == q + 1 || nlast < nfirst)
 	    break;
+	  q = end;
 	}
       for (; nfirst <= nlast; nfirst++)
 	{
@@ -413,16 +418,18 @@  gomp_affinity_init_numa_domains (unsigne
 		  bool seen = false;
 
 		  errno = 0;
-		  first = strtoul (p, &p, 10);
-		  if (errno)
+		  first = strtoul (p, &end, 10);
+		  if (errno || end == p)
 		    break;
+		  p = end;
 		  last = first;
 		  if (*p == '-')
 		    {
 		      errno = 0;
-		      last = strtoul (p + 1, &p, 10);
-		      if (errno || last < first)
+		      last = strtoul (p + 1, &end, 10);
+		      if (errno || end == p + 1 || last < first)
 			break;
+		      p = end;
 		    }
 		  for (; first <= last; first++)
 		    {
--- libgomp/config/rtems/proc.c.jj	2021-01-05 00:13:58.255297642 +0100
+++ libgomp/config/rtems/proc.c	2021-10-15 14:12:38.980134056 +0200
@@ -78,22 +78,25 @@  parse_thread_pools (char *env, unsigned
 {
   size_t len;
   int i;
+  char *end;
 
   if (*env == ':')
     ++env;
 
   errno = 0;
-  *count = strtoul (env, &env, 10);
-  if (errno != 0)
+  *count = strtoul (env, &end, 10);
+  if (errno != 0 || end == env)
     gomp_fatal ("Invalid thread pool count");
+  env = end;
 
   if (*env == '$')
     {
       ++env;
       errno = 0;
-      *priority = strtoul (env, &env, 10);
-      if (errno != 0)
+      *priority = strtoul (env, &end, 10);
+      if (errno != 0 || end == env)
 	gomp_fatal ("Invalid thread pool priority");
+      env = end;
     }
   else
     *priority = -1;