| Message ID | 1500568290-7966-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
On 07/20/2017 11:31 AM, Peter Maydell wrote: > On NetBSD, where tolower() and toupper() are implemented using an > array lookup, the compiler warns if you pass a plain 'char' > to these functions: > > gdbstub.c:914:13: warning: array subscript has type 'char' > > This reflects the fact that toupper() and tolower() give > undefined behaviour if they are passed a value that isn't > a valid 'unsigned char' or EOF. > > We have qemu_tolower() and qemu_toupper() to avoid this problem; > use them. > > (The use in scsi-generic.c does not trigger the warning because > it passes a uint8_t; we switch it anyway, for consistency.) > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 07/20/2017 06:31 PM, Peter Maydell wrote: > On NetBSD, where tolower() and toupper() are implemented using an > array lookup, the compiler warns if you pass a plain 'char' > to these functions: > > gdbstub.c:914:13: warning: array subscript has type 'char' > > This reflects the fact that toupper() and tolower() give > undefined behaviour if they are passed a value that isn't > a valid 'unsigned char' or EOF. > > We have qemu_tolower() and qemu_toupper() to avoid this problem; > use them. > > (The use in scsi-generic.c does not trigger the warning because > it passes a uint8_t; we switch it anyway, for consistency.) > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> for the s390 part. > --- > gdbstub.c | 2 +- > hw/s390x/s390-virtio-ccw.c | 2 +- > hw/scsi/scsi-generic.c | 2 +- > target/ppc/monitor.c | 4 ++-- > 4 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index f936ddd..2a94030 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -911,7 +911,7 @@ static int gdb_handle_vcont(GDBState *s, const char *p) > > cur_action = *p++; > if (cur_action == 'C' || cur_action == 'S') { > - cur_action = tolower(cur_action); > + cur_action = qemu_tolower(cur_action); > res = qemu_strtoul(p + 1, &p, 16, &tmp); > if (res) { > goto out; > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index ce3921e..1c7af39 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -318,7 +318,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp) > int i; > > for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) { > - uint8_t c = toupper(val[i]); /* mimic HMC */ > + uint8_t c = qemu_toupper(val[i]); /* mimic HMC */ > > if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || (c == '.') || > (c == ' ')) { > diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c > index a55ff87..7e1cbab 100644 > --- a/hw/scsi/scsi-generic.c > +++ b/hw/scsi/scsi-generic.c > @@ -406,7 +406,7 @@ static int read_naa_id(const uint8_t *p, uint64_t *p_wwn) > } > *p_wwn = 0; > for (i = 8; i < 24; i++) { > - char c = toupper(p[i]); > + char c = qemu_toupper(p[i]); > c -= (c >= '0' && c <= '9' ? '0' : 'A' - 10); > *p_wwn = (*p_wwn << 4) | c; > } > diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c > index b8f30e9..1491511 100644 > --- a/target/ppc/monitor.c > +++ b/target/ppc/monitor.c > @@ -115,14 +115,14 @@ int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval) > CPUPPCState *env = &cpu->env; > > /* General purpose registers */ > - if ((tolower(name[0]) == 'r') && > + if ((qemu_tolower(name[0]) == 'r') && > ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->gpr), ®num)) { > *pval = env->gpr[regnum]; > return 0; > } > > /* Floating point registers */ > - if ((tolower(name[0]) == 'f') && > + if ((qemu_tolower(name[0]) == 'f') && > ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->fpr), ®num)) { > *pval = env->fpr[regnum]; > return 0; >
On 07/20/2017 06:31 AM, Peter Maydell wrote: > gdbstub.c:914:13: warning: array subscript has type 'char' > > This reflects the fact that toupper() and tolower() give > undefined behaviour if they are passed a value that isn't > a valid 'unsigned char' or EOF. Not saying we shouldn't use qemu_tolower etc, but this statement is not true at all. Officially, the argument to toupper and tolower is type int. This sounds like a bug in NetBSD -- though it may not even be that, as they may have done something clever and put the symbol in the middle of the data. A trick that worked before compiler warnings got smarter. Anyway, should we poison the iso name so this doesn't creep in again? Reviewed-by: Richard Henderson <rth@twiddle.net> r~
On 07/20/2017 01:26 PM, Richard Henderson wrote: > On 07/20/2017 06:31 AM, Peter Maydell wrote: >> gdbstub.c:914:13: warning: array subscript has type 'char' >> >> This reflects the fact that toupper() and tolower() give >> undefined behaviour if they are passed a value that isn't >> a valid 'unsigned char' or EOF. > > Not saying we shouldn't use qemu_tolower etc, but this statement is not > true at all. Officially, the argument to toupper and tolower is type int. Correct. Officially, the argument to toupper is an int, but range-constrained to the set of ints that represent either EOF or an unsigned char value. Calling toupper(char) (on platforms where char is signed) is thus undefined behavior for chars < 0 (and which happen to not coincide with EOF). > > This sounds like a bug in NetBSD -- though it may not even be that, as > they may have done something clever and put the symbol in the middle of > the data. A trick that worked before compiler warnings got smarter. No, it is intentional of NetBSD. In fact, the Cygwin environment also INTENTIONALLY uses C99 magic to make gcc complain about calling toupper(char) while being silent on toupper(int) and toupper(unsigned char). Because you really DO have portability problems (even if toupper(char) _happens_ to work on most platforms, even where char is signed, does not make it portable). In fact, I'd love it if glibc would also adopt the appropriate gcc magic to warn about nonportable usage of ctype functions on signed char. From Cygwin's ctype.h: /* These macros are intentionally written in a manner that will trigger a gcc -Wall warning if the user mistakenly passes a 'char' instead of an int containing an 'unsigned char'. Note that the sizeof will always be 1, which is what we want for mapping EOF to __CTYPE_PTR[0]; the use of a raw index inside the sizeof triggers the gcc warning if __c was of type char, and sizeof masks side effects of the extra __c. Meanwhile, the real index to __CTYPE_PTR+1 must be cast to int, since isalpha(0x100000001LL) must equal isalpha(1), rather than being an out-of-bounds reference on a 64-bit machine. */ #define __ctype_lookup(__c) ((__CTYPE_PTR+sizeof(""[__c]))[(int)(__c)]) #define isalpha(__c) (__ctype_lookup(__c)&(_U|_L)) > > Anyway, should we poison the iso name so this doesn't creep in again? Yes, for ALL of the ctype names where we work around the issue (and maybe we should work around the issue in more places?) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 07/20/2017 08:48 AM, Eric Blake wrote: > On 07/20/2017 01:26 PM, Richard Henderson wrote: >> On 07/20/2017 06:31 AM, Peter Maydell wrote: >>> gdbstub.c:914:13: warning: array subscript has type 'char' >>> >>> This reflects the fact that toupper() and tolower() give >>> undefined behaviour if they are passed a value that isn't >>> a valid 'unsigned char' or EOF. >> >> Not saying we shouldn't use qemu_tolower etc, but this statement is not >> true at all. Officially, the argument to toupper and tolower is type int. > > Correct. Officially, the argument to toupper is an int, but > range-constrained to the set of ints that represent either EOF or an > unsigned char value. Please show me that language in C11 (or any other version). My copy has: 7.4.2.2 The toupper function Synopsis 1 #include <ctype.h> int toupper(int c); Description 2 The toupper function converts a lowercase letter to a corresponding uppercase letter. Returns 3 If the argument is a character for which islower is true and there are one or more corresponding characters, as specified by the current locale, for which isupper is true, the toupper function returns one of the corresponding characters (always the same one for any given locale); otherwise, the argument is returned unchanged. I see nothing about range constraints. The word "unsigned" does not appear. r~
On 07/20/2017 01:57 PM, Richard Henderson wrote: > On 07/20/2017 08:48 AM, Eric Blake wrote: >> On 07/20/2017 01:26 PM, Richard Henderson wrote: >>> On 07/20/2017 06:31 AM, Peter Maydell wrote: >>>> gdbstub.c:914:13: warning: array subscript has type 'char' >>>> >>>> This reflects the fact that toupper() and tolower() give >>>> undefined behaviour if they are passed a value that isn't >>>> a valid 'unsigned char' or EOF. >>> >>> Not saying we shouldn't use qemu_tolower etc, but this statement is not >>> true at all. Officially, the argument to toupper and tolower is type >>> int. >> >> Correct. Officially, the argument to toupper is an int, but >> range-constrained to the set of ints that represent either EOF or an >> unsigned char value. > > Please show me that language in C11 (or any other version). POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/toupper.html > For toupper(): [CX] [Option Start] The functionality described on this reference page is aligned with the ISO C standard. Any conflict between the requirements described here and the ISO C standard is unintentional. This volume of POSIX.1-2008 defers to the ISO C standard. [Option End] > > The toupper() [CX] [Option Start] and toupper_l() [Option End] functions have as a domain a type int, the value of which is representable as an unsigned char or the value of EOF. If the argument has any other value, the behavior is undefined. So POSIX is claiming it is a C99 requirement. And I found this in C11: 7.4 Character handling <ctype.h> 1 The header <ctype.h> declares several functions useful for classifying and mapping characters. 198) In all cases the argument is an int, the value of which shall be representable as an unsigned char or shall equal the value of the macro EOF. If the argument has any other value, the behavior is undefined. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 07/20/2017 09:04 AM, Eric Blake wrote: > And I found this in C11: > > 7.4 Character handling <ctype.h> > 1 The header <ctype.h> declares several functions useful for classifying > and mapping > characters. 198) In all cases the argument is an int, the value of which > shall be > representable as an unsigned char or shall equal the value of the macro > EOF. If the > argument has any other value, the behavior is undefined. Thanks. I should have looked up there. r~
On 20 July 2017 at 19:26, Richard Henderson <rth@twiddle.net> wrote: > On 07/20/2017 06:31 AM, Peter Maydell wrote: >> >> gdbstub.c:914:13: warning: array subscript has type 'char' >> >> This reflects the fact that toupper() and tolower() give >> undefined behaviour if they are passed a value that isn't >> a valid 'unsigned char' or EOF. > > > Not saying we shouldn't use qemu_tolower etc, but this statement is not true > at all. Officially, the argument to toupper and tolower is type int. (I was going to quote POSIX here but I see Eric has done so already.) > This sounds like a bug in NetBSD -- though it may not even be that, as they > may have done something clever and put the symbol in the middle of the data. > A trick that worked before compiler warnings got smarter. https://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/sys/ctype_inline.h The implementation is #define toupper(c) ((int)((_toupper_tab_ + 1)[(c)])) (where I assume _toupper_tab_ is the obvious array.) thanks -- PMM
On 07/20/2017 04:03 PM, Peter Maydell wrote: > On 20 July 2017 at 19:26, Richard Henderson <rth@twiddle.net> wrote: >> On 07/20/2017 06:31 AM, Peter Maydell wrote: >>> >>> gdbstub.c:914:13: warning: array subscript has type 'char' >>> >>> This reflects the fact that toupper() and tolower() give >>> undefined behaviour if they are passed a value that isn't >>> a valid 'unsigned char' or EOF. >> >> >> Not saying we shouldn't use qemu_tolower etc, but this statement is not true >> at all. Officially, the argument to toupper and tolower is type int. > > (I was going to quote POSIX here but I see Eric has done so already.) > >> This sounds like a bug in NetBSD -- though it may not even be that, as they >> may have done something clever and put the symbol in the middle of the data. >> A trick that worked before compiler warnings got smarter. > > https://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/sys/ctype_inline.h > The implementation is > #define toupper(c) ((int)((_toupper_tab_ + 1)[(c)])) > > (where I assume _toupper_tab_ is the obvious array.) In fact, if NetBSD's implementation is anything like Cygwin's, then it is a safe bet that _toupper_tab_ is a symbol that lands in the middle of a larger 384-byte array, such that _toupper_tab_[-1] (for (signed char)'\xfe') happens to resolve to a valid image address, and will have the same contents as _toupper_tab_[255] (for (unsigned char)'\xfe'), precisely to cater to so many clueless programs have forgotten about signed char widening to negative values (even though the standard says the behavior is undefined, quality-of-implementation demands that you try to do the sane thing anyway). The table is of course only MOSTLY symmetric; there are some single-byte locales where isspace((unsigned char)'\xff') is true but isspace(EOF) is false (in using the magic-middle-of-array locations, _tab_[0] cannot always match _tab_[256] in all locales). And it is wraparound where '\xff' collides with -1 that explains WHY C99 documents the range to be ints corresponding to unsigned char. Side note: NetBSD has a bug in their ctype.h. toupper(INT64_C(0x100000001)) is supposed to be the same as toupper(1), since POSIX requires toupper() to be available as a function (and permits a macro as well). A function with an int parameter must gracefully truncate a 64-bit input down to 32-bits, so the macro must do the same; but the NetBSD macro uses all 64 bits to dereference into garbage memory locations. Fortunately, no one runs into the bug in real life because no one really tries to pass 64-bit numbers to ctype macros. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 20 July 2017 at 22:29, Eric Blake <eblake@redhat.com> wrote: > On 07/20/2017 04:03 PM, Peter Maydell wrote: >> https://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/sys/ctype_inline.h >> The implementation is >> #define toupper(c) ((int)((_toupper_tab_ + 1)[(c)])) >> >> (where I assume _toupper_tab_ is the obvious array.) > > In fact, if NetBSD's implementation is anything like Cygwin's, then it > is a safe bet that _toupper_tab_ is a symbol that lands in the middle of > a larger 384-byte array, such that _toupper_tab_[-1] (for (signed > char)'\xfe') happens to resolve to a valid image address, and will have > the same contents as _toupper_tab_[255] (for (unsigned char)'\xfe'), > precisely to cater to so many clueless programs have forgotten about > signed char widening to negative values (even though the standard says > the behavior is undefined, quality-of-implementation demands that you > try to do the sane thing anyway). Nope, looks like they just implement as _toupper_tab_[0] is for EOF, and indexes >0 are for the characters, if I've found the right bit of their libc: https://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/lib/libc/locale/rune.c thanks -- PMM
On Thu, Jul 20, 2017 at 05:31:30PM +0100, Peter Maydell wrote: > On NetBSD, where tolower() and toupper() are implemented using an > array lookup, the compiler warns if you pass a plain 'char' > to these functions: > > gdbstub.c:914:13: warning: array subscript has type 'char' > > This reflects the fact that toupper() and tolower() give > undefined behaviour if they are passed a value that isn't > a valid 'unsigned char' or EOF. > > We have qemu_tolower() and qemu_toupper() to avoid this problem; > use them. > > (The use in scsi-generic.c does not trigger the warning because > it passes a uint8_t; we switch it anyway, for consistency.) > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> ppc parts Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > gdbstub.c | 2 +- > hw/s390x/s390-virtio-ccw.c | 2 +- > hw/scsi/scsi-generic.c | 2 +- > target/ppc/monitor.c | 4 ++-- > 4 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index f936ddd..2a94030 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -911,7 +911,7 @@ static int gdb_handle_vcont(GDBState *s, const char *p) > > cur_action = *p++; > if (cur_action == 'C' || cur_action == 'S') { > - cur_action = tolower(cur_action); > + cur_action = qemu_tolower(cur_action); > res = qemu_strtoul(p + 1, &p, 16, &tmp); > if (res) { > goto out; > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index ce3921e..1c7af39 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -318,7 +318,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp) > int i; > > for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) { > - uint8_t c = toupper(val[i]); /* mimic HMC */ > + uint8_t c = qemu_toupper(val[i]); /* mimic HMC */ > > if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || (c == '.') || > (c == ' ')) { > diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c > index a55ff87..7e1cbab 100644 > --- a/hw/scsi/scsi-generic.c > +++ b/hw/scsi/scsi-generic.c > @@ -406,7 +406,7 @@ static int read_naa_id(const uint8_t *p, uint64_t *p_wwn) > } > *p_wwn = 0; > for (i = 8; i < 24; i++) { > - char c = toupper(p[i]); > + char c = qemu_toupper(p[i]); > c -= (c >= '0' && c <= '9' ? '0' : 'A' - 10); > *p_wwn = (*p_wwn << 4) | c; > } > diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c > index b8f30e9..1491511 100644 > --- a/target/ppc/monitor.c > +++ b/target/ppc/monitor.c > @@ -115,14 +115,14 @@ int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval) > CPUPPCState *env = &cpu->env; > > /* General purpose registers */ > - if ((tolower(name[0]) == 'r') && > + if ((qemu_tolower(name[0]) == 'r') && > ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->gpr), ®num)) { > *pval = env->gpr[regnum]; > return 0; > } > > /* Floating point registers */ > - if ((tolower(name[0]) == 'f') && > + if ((qemu_tolower(name[0]) == 'f') && > ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->fpr), ®num)) { > *pval = env->fpr[regnum]; > return 0; -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 20 July 2017 at 17:31, Peter Maydell <peter.maydell@linaro.org> wrote: > On NetBSD, where tolower() and toupper() are implemented using an > array lookup, the compiler warns if you pass a plain 'char' > to these functions: > > gdbstub.c:914:13: warning: array subscript has type 'char' > > This reflects the fact that toupper() and tolower() give > undefined behaviour if they are passed a value that isn't > a valid 'unsigned char' or EOF. > > We have qemu_tolower() and qemu_toupper() to avoid this problem; > use them. > > (The use in scsi-generic.c does not trigger the warning because > it passes a uint8_t; we switch it anyway, for consistency.) > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Applied to master; thanks all for fast reviews/acks. -- PMM
diff --git a/gdbstub.c b/gdbstub.c index f936ddd..2a94030 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -911,7 +911,7 @@ static int gdb_handle_vcont(GDBState *s, const char *p) cur_action = *p++; if (cur_action == 'C' || cur_action == 'S') { - cur_action = tolower(cur_action); + cur_action = qemu_tolower(cur_action); res = qemu_strtoul(p + 1, &p, 16, &tmp); if (res) { goto out; diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index ce3921e..1c7af39 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -318,7 +318,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp) int i; for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) { - uint8_t c = toupper(val[i]); /* mimic HMC */ + uint8_t c = qemu_toupper(val[i]); /* mimic HMC */ if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || (c == '.') || (c == ' ')) { diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index a55ff87..7e1cbab 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -406,7 +406,7 @@ static int read_naa_id(const uint8_t *p, uint64_t *p_wwn) } *p_wwn = 0; for (i = 8; i < 24; i++) { - char c = toupper(p[i]); + char c = qemu_toupper(p[i]); c -= (c >= '0' && c <= '9' ? '0' : 'A' - 10); *p_wwn = (*p_wwn << 4) | c; } diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c index b8f30e9..1491511 100644 --- a/target/ppc/monitor.c +++ b/target/ppc/monitor.c @@ -115,14 +115,14 @@ int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval) CPUPPCState *env = &cpu->env; /* General purpose registers */ - if ((tolower(name[0]) == 'r') && + if ((qemu_tolower(name[0]) == 'r') && ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->gpr), ®num)) { *pval = env->gpr[regnum]; return 0; } /* Floating point registers */ - if ((tolower(name[0]) == 'f') && + if ((qemu_tolower(name[0]) == 'f') && ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->fpr), ®num)) { *pval = env->fpr[regnum]; return 0;
On NetBSD, where tolower() and toupper() are implemented using an array lookup, the compiler warns if you pass a plain 'char' to these functions: gdbstub.c:914:13: warning: array subscript has type 'char' This reflects the fact that toupper() and tolower() give undefined behaviour if they are passed a value that isn't a valid 'unsigned char' or EOF. We have qemu_tolower() and qemu_toupper() to avoid this problem; use them. (The use in scsi-generic.c does not trigger the warning because it passes a uint8_t; we switch it anyway, for consistency.) Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- gdbstub.c | 2 +- hw/s390x/s390-virtio-ccw.c | 2 +- hw/scsi/scsi-generic.c | 2 +- target/ppc/monitor.c | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) -- 2.7.4