Philippe Mathieu-Daudé <phi...@redhat.com> writes: > On 4/18/19 4:53 PM, Markus Armbruster wrote: >> parse_acl_file() passes char values to isspace(). Undefined behavior >> when the value is negative. Not a security issue, because the >> characters come from trusted $prefix/etc/qemu/bridge.conf and the >> files it includes. >> >> Fix by using qemu_isspace() instead. > > Can we use g_ascii_isspace() and remove qemu_isspace()?
Hmm, I wasn't aware of that one. arg type arg values obeys locale? ctype.h isFOO() int unsigned char, EOF [1] yes [4] qemu_isFOO() int any char [2] yes [4] g_ascii_isFOO() char any char [3] no [1] Undefined behavior when the argument is a negative plain or signed char. Well-known trap. [2] qemu_isFOO(arg) expands into isFOO((unsigned char)arg), which works fine for plain, signed and unsigned char arg. [3] When plain char is signed, and the actual argument is unsigned char and not representable as char, then behavior is implementation-defined. No problem; the implementations we care for reinterpret as two's complement. [4] Obeying the locale is commonly unwanted. Replacing isFOO() by qemu_isFOO() gets rid of trap [1] (and loses EOF, but that rarely matters). Replacing qemu_isFOO() by g_ascii_isFOO() gets rid of the commonly unwanted locale dependence. Each such replacement needs review, no matter how common "unwanted" may be. I suspect we'd almost always be better off with g_ascii_isFOO() instead of qemu_isFOO(). If that's the case, then the few exceptions (if any) could use standard isFOO(), so we can drop qemu_isFOO(). I'd rather not do that myself globally now due to the "needs review" part. Perhaps I should do it just for this file while I touch it anyway. The question to ask: should parse_acl_file() obey the locale for whitespace recognition? >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> qemu-bridge-helper.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c >> index 5396fbfbb6..0d60c07655 100644 >> --- a/qemu-bridge-helper.c >> +++ b/qemu-bridge-helper.c >> @@ -29,6 +29,7 @@ >> #include <linux/if_bridge.h> >> #endif >> >> +#include "qemu-common.h" >> #include "qemu/queue.h" >> >> #include "net/tap-linux.h" >> @@ -75,7 +76,7 @@ static int parse_acl_file(const char *filename, ACLList >> *acl_list) >> char *ptr = line; >> char *cmd, *arg, *argend; >> >> - while (isspace(*ptr)) { >> + while (qemu_isspace(*ptr)) { >> ptr++; >> } >> >> @@ -99,12 +100,12 @@ static int parse_acl_file(const char *filename, ACLList >> *acl_list) >> >> *arg = 0; >> arg++; >> - while (isspace(*arg)) { >> + while (qemu_isspace(*arg)) { >> arg++; >> } >> >> argend = arg + strlen(arg); >> - while (arg != argend && isspace(*(argend - 1))) { >> + while (arg != argend && qemu_isspace(*(argend - 1))) { >> argend--; >> } >> *argend = 0; >>