From 3b93a42702dd4e8b587b1a1d755a78c1354814f5 Mon Sep 17 00:00:00 2001 From: Thales Antunes de Oliveira Barretto Date: Thu, 20 Feb 2025 01:12:43 -0300 Subject: [PATCH] Fix likely misuse of ctype.h API #4397 The standard C API takes type int as argument, but when that argument value is neither (a) a value representable by type unsigned char nor (b) the value of the macro EOF, the behaviour is undefined. See C11, Sec. 7.4 Character handling p.200, clause 1. The functions are designed to take as arguments the values returned by getc or fgetc, no for processing elements of an arbitrary string stored in a char array. Safely processing arbitrary strings requires explicit cast to unsigned char to keep the argument values within the domain {EOF, 0, 1 ..., 255}. OTH, passing negative values {-128, -127, ..., -1} may trigger undefined behaviour, and also the non-EOF 0xff can be conflated with -1, which, although not forbidden, may give unintended results. Casting to unsigned char avoids sign extension when implicitly converting to int. This commit introduces an explicit cast to unsigned char when passing argument to the standard function family. Reported-by: riastradh Signed-off-by: Thales Antunes de Oliveira Barretto --- api/src/glfs.c | 2 +- cli/src/cli-cmd-parser.c | 12 +++---- cli/src/cli-xml-output.c | 4 +-- extras/benchmarking/rdd.c | 2 +- extras/geo-rep/gsync-sync-gfid.c | 2 +- geo-replication/src/procdiggy.c | 4 +-- libglusterfs/src/common-utils.c | 31 ++++++++++--------- tests/basic/open-behind/tester.c | 5 +-- xlators/cluster/ec/src/ec-code.c | 4 +-- xlators/debug/io-stats/src/io-stats.c | 4 +-- xlators/mgmt/glusterd/src/glusterd-ganesha.c | 5 +-- xlators/mgmt/glusterd/src/glusterd-geo-rep.c | 10 +++--- .../mgmt/glusterd/src/glusterd-mountbroker.c | 4 +-- xlators/mgmt/glusterd/src/glusterd-pmap.c | 4 +-- xlators/mgmt/glusterd/src/glusterd-utils.c | 2 +- .../mgmt/glusterd/src/glusterd-volume-set.c | 3 +- xlators/performance/md-cache/src/md-cache.c | 2 +- 17 files changed, 53 insertions(+), 47 deletions(-) diff --git a/api/src/glfs.c b/api/src/glfs.c index 1a4bfc14343..af184e5cadc 100644 --- a/api/src/glfs.c +++ b/api/src/glfs.c @@ -850,7 +850,7 @@ pub_glfs_new(const char *volname) } for (i = 0; i < strlen(volname); i++) { - if (!isalnum(volname[i]) && (volname[i] != '_') && + if (!isalnum((unsigned char)volname[i]) && (volname[i] != '_') && (volname[i] != '-')) { errno = EINVAL; return NULL; diff --git a/cli/src/cli-cmd-parser.c b/cli/src/cli-cmd-parser.c index fe5864b0eb8..6d89855ea94 100644 --- a/cli/src/cli-cmd-parser.c +++ b/cli/src/cli-cmd-parser.c @@ -562,7 +562,7 @@ cli_validate_volname(const char *volname) } for (i = 0; i < volname_len; i++) { - if (!isalnum(volname[i]) && (volname[i] != '_') && + if (!isalnum((unsigned char)volname[i]) && (volname[i] != '_') && (volname[i] != '-')) { cli_err( "Volume name should not contain \"%c\"" @@ -1702,7 +1702,7 @@ gf_strip_whitespace(char *str, int len) return -1; for (i = 0; i < len; i++) { - if (!isspace(str[i])) + if (!isspace((unsigned char)str[i])) new_str[new_len++] = str[i]; } new_str[new_len] = '\0'; @@ -3087,7 +3087,7 @@ gf_is_str_int(const char *value) fptr = str; while (*str) { - if (!isdigit(*str)) { + if (!isdigit((unsigned char)*str)) { flag = 1; goto out; } @@ -4173,7 +4173,7 @@ cli_snap_clone_parse(dict_t *dict, const char **words, int wordcount) clonename = (char *)words[cmdi]; for (i = 0; i < strlen(clonename); i++) { /* Following volume name convention */ - if (!isalnum(clonename[i]) && + if (!isalnum((unsigned char)clonename[i]) && (clonename[i] != '_' && (clonename[i] != '-'))) { /* TODO : Is this message enough?? */ cli_err( @@ -4255,7 +4255,7 @@ cli_snap_create_parse(dict_t *dict, const char **words, int wordcount) snapname = (char *)words[cmdi]; for (i = 0; i < strlen(snapname); i++) { /* Following volume name convention */ - if (!isalnum(snapname[i]) && + if (!isalnum((unsigned char)snapname[i]) && (snapname[i] != '_' && (snapname[i] != '-'))) { /* TODO : Is this message enough?? */ cli_err( @@ -5468,7 +5468,7 @@ cli_cmd_validate_volume(char *volname) } for (i = 0; i < volname_len; i++) - if (!isalnum(volname[i]) && (volname[i] != '_') && + if (!isalnum((unsigned char)volname[i]) && (volname[i] != '_') && (volname[i] != '-')) { cli_err( "Volume name should not contain \"%c\"" diff --git a/cli/src/cli-xml-output.c b/cli/src/cli-xml-output.c index 1cdf28bcbfe..b5b08998565 100644 --- a/cli/src/cli-xml-output.c +++ b/cli/src/cli-xml-output.c @@ -3412,7 +3412,7 @@ _output_gsync_config(FILE *fp, xmlTextWriterPtr writer, char *op_name) break; v = resbuf + strlen(resbuf) - 1; - while (isspace(*v)) { + while (isspace((unsigned char)*v)) { /* strip trailing space */ *v-- = '\0'; } @@ -3434,7 +3434,7 @@ _output_gsync_config(FILE *fp, xmlTextWriterPtr writer, char *op_name) goto out; } *v++ = '\0'; - while (isspace(*v)) + while (isspace((unsigned char)*v)) v++; v = gf_strdup(v); if (!v) { diff --git a/extras/benchmarking/rdd.c b/extras/benchmarking/rdd.c index efc9d342a37..18a7547d3ca 100644 --- a/extras/benchmarking/rdd.c +++ b/extras/benchmarking/rdd.c @@ -209,7 +209,7 @@ string2bytesize(const char *str, unsigned long long *n) } for (s = str; *s != '\0'; s++) { - if (isspace(*s)) { + if (isspace((unsigned char)*s)) { continue; } if (*s == '-') { diff --git a/extras/geo-rep/gsync-sync-gfid.c b/extras/geo-rep/gsync-sync-gfid.c index 80c50b2c232..da25f38e485 100644 --- a/extras/geo-rep/gsync-sync-gfid.c +++ b/extras/geo-rep/gsync-sync-gfid.c @@ -62,7 +62,7 @@ main(int argc, char *argv[]) path += UUID_CANONICAL_FORM_LEN + 1; - while (isspace(*path)) + while (isspace((unsigned char)*path)) path++; len = strlen(line); diff --git a/geo-replication/src/procdiggy.c b/geo-replication/src/procdiggy.c index 8068ef79a42..0110fe64dd0 100644 --- a/geo-replication/src/procdiggy.c +++ b/geo-replication/src/procdiggy.c @@ -55,7 +55,7 @@ pidinfo(pid_t pid, char **name) if (name && !*name) { p = strtail(buf, "Name:"); if (p) { - while (isspace(*++p)) + while (isspace((unsigned char)*++p)) ; *name = gf_strdup(p); if (!*name) { @@ -71,7 +71,7 @@ pidinfo(pid_t pid, char **name) break; } - while (isspace(*++p)) + while (isspace((unsigned char)*++p)) ; ret = gf_string2int(p, &lpid); if (ret == -1) diff --git a/libglusterfs/src/common-utils.c b/libglusterfs/src/common-utils.c index 01c8a4ed14e..3a76d4f8bf0 100644 --- a/libglusterfs/src/common-utils.c +++ b/libglusterfs/src/common-utils.c @@ -713,14 +713,14 @@ gf_trim(char *string) return NULL; } - for (s = string; isspace(*s); s++) + for (s = string; isspace((unsigned char)*s); s++) ; if (*s == 0) return s; t = s + strlen(s) - 1; - while (t > s && isspace(*t)) + while (t > s && isspace((unsigned char)*t)) t--; *++t = '\0'; @@ -778,7 +778,7 @@ gf_string2time(const char *str, time_t *n) } for (s = str; *s != '\0'; s++) { - if (isspace(*s)) + if (isspace((unsigned char)*s)) continue; if (*s == '-') return -1; @@ -852,7 +852,7 @@ gf_string2percent(const char *str, double *n) } for (s = str; *s != '\0'; s++) { - if (isspace(*s)) + if (isspace((unsigned char)*s)) continue; if (*s == '-') return -1; @@ -928,7 +928,7 @@ _gf_string2ulong(const char *str, unsigned long *n, int base) } for (s = str; *s != '\0'; s++) { - if (isspace(*s)) + if (isspace((unsigned char)*s)) continue; if (*s == '-') return -1; @@ -971,7 +971,7 @@ _gf_string2uint(const char *str, unsigned int *n, int base) } for (s = str; *s != '\0'; s++) { - if (isspace(*s)) + if (isspace((unsigned char)*s)) continue; if (*s == '-') return -1; @@ -1082,7 +1082,7 @@ _gf_string2ulonglong(const char *str, unsigned long long *n, int base) } for (s = str; *s != '\0'; s++) { - if (isspace(*s)) + if (isspace((unsigned char)*s)) continue; if (*s == '-') return -1; @@ -1450,7 +1450,7 @@ gf_string2bytesize_range(const char *str, uint64_t *n, uint64_t umax) max = umax & 0x7fffffffffffffffLL; for (s = str; *s != '\0'; s++) { - if (isspace(*s)) + if (isspace((unsigned char)*s)) continue; if (*s == '-') return -1; @@ -1549,7 +1549,7 @@ gf_string2percent_or_bytesize(const char *str, double *n, } for (s = str; *s != '\0'; s++) { - if (isspace(*s)) + if (isspace((unsigned char)*s)) continue; if (*s == '-') return -1; @@ -1806,7 +1806,7 @@ strtail(char *str, const char *pattern) void skipwhite(char **s) { - while (isspace(**s)) + while (isspace((unsigned char)**s)) (*s)++; } @@ -1995,7 +1995,8 @@ valid_host_name(char *address, int length) goto out; } - if (!isalnum(dup_addr[length - 1]) && (dup_addr[length - 1] != '*')) { + if (!isalnum((unsigned char)dup_addr[length - 1]) && + (dup_addr[length - 1] != '*')) { ret = 0; goto out; } @@ -2013,12 +2014,13 @@ valid_host_name(char *address, int length) do { str_len = strlen(temp_str); - if (!isalnum(temp_str[0]) || !isalnum(temp_str[str_len - 1])) { + if (!isalnum((unsigned char)temp_str[0]) || + !isalnum((unsigned char)temp_str[str_len - 1])) { ret = 0; goto out; } for (i = 1; i < str_len; i++) { - if (!isalnum(temp_str[i]) && (temp_str[i] != '-')) { + if (!isalnum((unsigned char)temp_str[i]) && (temp_str[i] != '-')) { ret = 0; goto out; } @@ -2049,7 +2051,8 @@ valid_ipv4_address(char *address, int length, gf_boolean_t wildcard_acc) * delimiters. */ if (length <= 0 || (strstr(address, "..")) || - (!isdigit(tmp[length - 1]) && (tmp[length - 1] != '*'))) { + (!isdigit((unsigned char)tmp[length - 1]) && + (tmp[length - 1] != '*'))) { ret = 0; goto out; } diff --git a/tests/basic/open-behind/tester.c b/tests/basic/open-behind/tester.c index b2da71c8385..1f6e4e8d39f 100644 --- a/tests/basic/open-behind/tester.c +++ b/tests/basic/open-behind/tester.c @@ -82,7 +82,8 @@ buffer_get(context_t *ctx) static int32_t str_skip_spaces(context_t *ctx, int32_t current) { - while ((current > 0) && (current != '\n') && isspace(current)) { + while ((current > 0) && (current != '\n') && + isspace((unsigned char)current)) { current = buffer_get(ctx); } @@ -98,7 +99,7 @@ str_token(context_t *ctx, char *buffer, uint32_t size, int32_t current) len = 0; while ((size > 0) && (current > 0) && (current != '\n') && - !isspace(current)) { + !isspace((unsigned char)current)) { len++; *buffer++ = current; size--; diff --git a/xlators/cluster/ec/src/ec-code.c b/xlators/cluster/ec/src/ec-code.c index 03162ae05a9..c5c14a8a437 100644 --- a/xlators/cluster/ec/src/ec-code.c +++ b/xlators/cluster/ec/src/ec-code.c @@ -838,7 +838,7 @@ ec_code_proc_trim_left(char *text, ssize_t *length) { ssize_t len; - for (len = *length; (len > 0) && isspace(*text); len--) { + for (len = *length; (len > 0) && isspace((unsigned char)*text); len--) { text++; } *length = len; @@ -856,7 +856,7 @@ ec_code_proc_trim_right(char *text, ssize_t *length, char sep) last = text; for (len = *length; (len > 0) && (*text != sep); len--) { - if (!isspace(*text)) { + if (!isspace((unsigned char)*text)) { last = text + 1; } text++; diff --git a/xlators/debug/io-stats/src/io-stats.c b/xlators/debug/io-stats/src/io-stats.c index 2ab2703c68f..0300d71a6d0 100644 --- a/xlators/debug/io-stats/src/io-stats.c +++ b/xlators/debug/io-stats/src/io-stats.c @@ -822,7 +822,7 @@ io_stats_dump_global_to_json_logfp(xlator_t *this, for (i = 0; i < GF_FOP_MAXVALUE; i++) { lc_fop_name = strdupa(gf_fop_list[i]); for (j = 0; lc_fop_name[j]; j++) { - lc_fop_name[j] = tolower(lc_fop_name[j]); + lc_fop_name[j] = tolower((unsigned char)lc_fop_name[j]); } fop_hits = GF_ATOMIC_GET(stats->fop_hits[i]); @@ -879,7 +879,7 @@ io_stats_dump_global_to_json_logfp(xlator_t *this, for (i = 0; i < GF_UPCALL_FLAGS_MAXVALUE; i++) { lc_fop_name = strdupa(gf_upcall_list[i]); for (j = 0; lc_fop_name[j]; j++) { - lc_fop_name[j] = tolower(lc_fop_name[j]); + lc_fop_name[j] = tolower((unsigned char)lc_fop_name[j]); } fop_hits = GF_ATOMIC_GET(stats->upcall_hits[i]); if (interval == -1) { diff --git a/xlators/mgmt/glusterd/src/glusterd-ganesha.c b/xlators/mgmt/glusterd/src/glusterd-ganesha.c index 36b62ede609..ab5e5c8fc2d 100644 --- a/xlators/mgmt/glusterd/src/glusterd-ganesha.c +++ b/xlators/mgmt/glusterd/src/glusterd-ganesha.c @@ -55,7 +55,7 @@ parsing_ganesha_ha_conf(const char *key) if (*pointer == '#') { continue; } - while (isblank(*pointer)) { + while (isblank((unsigned char)*pointer)) { pointer++; } if (strncmp(pointer, key, strlen(key))) { @@ -83,7 +83,8 @@ parsing_ganesha_ha_conf(const char *key) do { end_pointer++; } while (!(*end_pointer == '\'' || *end_pointer == '"' || - isspace(*end_pointer) || *end_pointer == '\0')); + isspace((unsigned char)*end_pointer) || + *end_pointer == '\0')); *end_pointer = '\0'; /* got it. copy it and return */ diff --git a/xlators/mgmt/glusterd/src/glusterd-geo-rep.c b/xlators/mgmt/glusterd/src/glusterd-geo-rep.c index 308a296fd29..2b592f38ec5 100644 --- a/xlators/mgmt/glusterd/src/glusterd-geo-rep.c +++ b/xlators/mgmt/glusterd/src/glusterd-geo-rep.c @@ -760,7 +760,7 @@ _fcbk_conftodict(char *resbuf, size_t blen, FILE *fp, void *data) if (!ptr) break; v = resbuf + strlen(resbuf) - 1; - while (isspace(*v)) + while (isspace((unsigned char)*v)) /* strip trailing space */ *v-- = '\0'; if (v == resbuf) @@ -770,7 +770,7 @@ _fcbk_conftodict(char *resbuf, size_t blen, FILE *fp, void *data) if (!v) return -1; *v++ = '\0'; - while (isspace(*v)) + while (isspace((unsigned char)*v)) v++; v = gf_strdup(v); if (!v) @@ -826,7 +826,7 @@ _fcbk_statustostruct(char *resbuf, size_t blen, FILE *fp, void *data) break; v = resbuf + strlen(resbuf) - 1; - while (isspace(*v)) + while (isspace((unsigned char)*v)) /* strip trailing space */ *v-- = '\0'; if (v == resbuf) @@ -836,7 +836,7 @@ _fcbk_statustostruct(char *resbuf, size_t blen, FILE *fp, void *data) if (!v) return -1; *v++ = '\0'; - while (isspace(*v)) + while (isspace((unsigned char)*v)) v++; v = gf_strdup(v); if (!v) @@ -4427,7 +4427,7 @@ glusterd_gsync_read_frm_status(char *path, char *buf, size_t blen) ret = -1; } else { char *p = buf + len - 1; - while (isspace(*p)) + while (isspace((unsigned char)*p)) *p-- = '\0'; } } else if (ret == 0) diff --git a/xlators/mgmt/glusterd/src/glusterd-mountbroker.c b/xlators/mgmt/glusterd/src/glusterd-mountbroker.c index 5b04e67a1de..9ba3191e564 100644 --- a/xlators/mgmt/glusterd/src/glusterd-mountbroker.c +++ b/xlators/mgmt/glusterd/src/glusterd-mountbroker.c @@ -140,7 +140,7 @@ parse_mount_pattern_desc(gf_mount_spec_t *mspec, char *pdesc) ret = SYNTAX_ERR; goto out; } - while (!strchr("|&)", *c2) && !isspace(*c2)) + while (!strchr("|&)", *c2) && !isspace((unsigned char)*c2)) c2++; skipwhite(&c2); switch (*c2) { @@ -182,7 +182,7 @@ parse_mount_pattern_desc(gf_mount_spec_t *mspec, char *pdesc) c2 = ""; /* reset c2 */ while (*c2 != ')') { c2 = curs; - while (!isspace(*c2) && *c2 != ')') + while (!isspace((unsigned char)*c2) && *c2 != ')') c2++; sc = *c2; *c2 = '\0'; diff --git a/xlators/mgmt/glusterd/src/glusterd-pmap.c b/xlators/mgmt/glusterd/src/glusterd-pmap.c index 39c65762338..ed5768bda3e 100644 --- a/xlators/mgmt/glusterd/src/glusterd-pmap.c +++ b/xlators/mgmt/glusterd/src/glusterd-pmap.c @@ -150,7 +150,7 @@ pmap_registry_search(xlator_t *this, char *brickname, gf_boolean_t destroy) { brck = tmp_port->brickname; for (;;) { - for (i = 0; brck[i] && !isspace(brck[i]); ++i) + for (i = 0; brck[i] && !isspace((unsigned char)brck[i]); ++i) ; if (i == 0 && brck[i] == '\0') break; @@ -175,7 +175,7 @@ pmap_registry_search(xlator_t *this, char *brickname, gf_boolean_t destroy) * Skip over *any* amount of whitespace, including * none (if we're already at the end of the string). */ - while (isspace(*brck)) + while (isspace((unsigned char)*brck)) ++brck; /* * We're either at the end of the string (which will be diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c index 824b03c4d8f..74ff216f660 100644 --- a/xlators/mgmt/glusterd/src/glusterd-utils.c +++ b/xlators/mgmt/glusterd/src/glusterd-utils.c @@ -6927,7 +6927,7 @@ glusterd_parse_inode_size(char *stream, char *pattern) needle = nwstrtail(needle, pattern); trail = needle; - while (trail && isdigit(*trail)) + while (trail && isdigit((unsigned char)*trail)) trail++; if (trail) *trail = '\0'; diff --git a/xlators/mgmt/glusterd/src/glusterd-volume-set.c b/xlators/mgmt/glusterd/src/glusterd-volume-set.c index 48232ed7f70..9296a7c9a99 100644 --- a/xlators/mgmt/glusterd/src/glusterd-volume-set.c +++ b/xlators/mgmt/glusterd/src/glusterd-volume-set.c @@ -175,7 +175,8 @@ validate_uss_dir(glusterd_volinfo_t *volinfo, dict_t *dict, char *key, } for (i = 1; value[i]; i++) { - if (isalnum(value[i]) || value[i] == '_' || value[i] == '-') + if (isalnum((unsigned char)value[i]) || value[i] == '_' || + value[i] == '-') continue; snprintf(errstr, sizeof(errstr), diff --git a/xlators/performance/md-cache/src/md-cache.c b/xlators/performance/md-cache/src/md-cache.c index 51c1a68c7a4..f3197d1df74 100644 --- a/xlators/performance/md-cache/src/md-cache.c +++ b/xlators/performance/md-cache/src/md-cache.c @@ -662,7 +662,7 @@ gf_strTrim(char **s) *(end + 1) = '\0'; - while (isspace(**s)) + while (isspace((unsigned char)**s)) (*s)++; return;