* [committed] openmp: Fix up strtoul and strtoull uses in libgomp
@ 2021-10-15 14:46 Jakub Jelinek
2021-11-09 16:06 ` Thomas Schwinge
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2021-10-15 14:46 UTC (permalink / raw)
To: gcc-patches; +Cc: Tobias Burnus
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.
--- 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;
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [committed] openmp: Fix up strtoul and strtoull uses in libgomp
2021-10-15 14:46 [committed] openmp: Fix up strtoul and strtoull uses in libgomp Jakub Jelinek
@ 2021-11-09 16:06 ` Thomas Schwinge
0 siblings, 0 replies; 2+ messages in thread
From: Thomas Schwinge @ 2021-11-09 16:06 UTC (permalink / raw)
To: Jakub Jelinek, gcc-patches; +Cc: Tobias Burnus
[-- Attachment #1: Type: text/plain, Size: 3208 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Restore-GOMP_OPENACC_DIM-environment-variable-parsin.patch --]
[-- Type: text/x-diff, Size: 1032 bytes --]
From 00c9ce13a64e324dabd8dfd236882919a3119479 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Fri, 5 Nov 2021 14:42:21 +0100
Subject: [PATCH] Restore 'GOMP_OPENACC_DIM' environment variable parsing
... that got broken by recent commit c057ed9c52c6a63a1a692268f916b1a9131cd4b7
"openmp: Fix up strtoul and strtoull uses in libgomp", resulting in spurious
FAILs for tests specifying 'dg-set-target-env-var "GOMP_OPENACC_DIM" "[...]"'.
libgomp/
* env.c (parse_gomp_openacc_dim): Restore parsing.
---
libgomp/env.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libgomp/env.c b/libgomp/env.c
index df10ff656b6..75018e8c252 100644
--- a/libgomp/env.c
+++ b/libgomp/env.c
@@ -1243,7 +1243,7 @@ parse_gomp_openacc_dim (void)
errno = 0;
val = strtol (pos, &eptr, 10);
- if (errno || eptr != pos || val < 0 || (unsigned)val != val)
+ if (errno || eptr == pos || val < 0 || (unsigned)val != val)
break;
goacc_default_dims[i] = (int)val;
--
2.33.0
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-11-09 16:07 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-15 14:46 [committed] openmp: Fix up strtoul and strtoull uses in libgomp Jakub Jelinek
2021-11-09 16:06 ` Thomas Schwinge
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).