Re: [Qemu-devel] [PATCH 1/6] qemu-bridge-helper: Fix misuse of isspace()

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
>> ---
>>  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;
>>