Philippe Mathieu-Daudé <[email protected]> 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 <[email protected]>
>> ---
>> 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;
>>
