| Message ID | 20190522134726.19225-2-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/5] qemu-bridge-helper: Fix misuse of isspace() | expand |
On Wed, 22 May 2019 at 14:49, Markus Armbruster <armbru@redhat.com> 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. > > Furthermore, isspace()'s locale-dependence means qemu-bridge-helper > uses the user's locale for parsing $prefix/etc/bridge.conf. Feels > wrong. > > Use g_ascii_isspace() instead. This fixes the undefined behavior, and > makes parsing of $prefix/etc/bridge.conf locale-independent. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Message-Id: <20190514180311.16028-2-armbru@redhat.com> > --- > qemu-bridge-helper.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Coverity complains about this change (CID 1401706) because it doesn't have enough information to know that the table lookup g_ascii_isspace does is always safe: tainted_data: Using tainted variable (guchar)*(argend - 1) as an index to pointer g_ascii_table. We know this is OK because we know the table is big enough that a guchar index can't possibly overrun, but because the table is declared in the glib header file as GLIB_VAR const guint16 * const g_ascii_table; Coverity has no idea of its size and is being pessimistic. I've squashed the Coverity issue as a false-positive, but I mention it here in case you thought it worth trying to write something in coverity-model.c to provide a better model of the glib function. thanks -- PMM
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c index 5396fbfbb6..f9940deefd 100644 --- a/qemu-bridge-helper.c +++ b/qemu-bridge-helper.c @@ -75,7 +75,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list) char *ptr = line; char *cmd, *arg, *argend; - while (isspace(*ptr)) { + while (g_ascii_isspace(*ptr)) { ptr++; } @@ -99,12 +99,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list) *arg = 0; arg++; - while (isspace(*arg)) { + while (g_ascii_isspace(*arg)) { arg++; } argend = arg + strlen(arg); - while (arg != argend && isspace(*(argend - 1))) { + while (arg != argend && g_ascii_isspace(*(argend - 1))) { argend--; } *argend = 0;
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. Furthermore, isspace()'s locale-dependence means qemu-bridge-helper uses the user's locale for parsing $prefix/etc/bridge.conf. Feels wrong. Use g_ascii_isspace() instead. This fixes the undefined behavior, and makes parsing of $prefix/etc/bridge.conf locale-independent. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-Id: <20190514180311.16028-2-armbru@redhat.com> --- qemu-bridge-helper.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)