| Message ID | 1428516988-6760-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/08/2015 12:16 PM, Eduardo Habkost wrote: > Currently it is impossible to set an option in a config file to an empty > string, because the parser matches only lines containing non-empty > strings between double-quotes. > > As sscanf() "[" conversion specifier only matches non-empty strings, add > a special case for empty strings. I avoid sscanf() as a rule (as it's behavior on %d is undefined in the face of malicious input), so I had to read the man page; but you are right. Libvirt is trying to completely ban use of *scanf for that reason; but obviously qemu is not quite so opposed to it. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > util/qemu-config.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/util/qemu-config.c b/util/qemu-config.c > index 2d32ce7..9f9577d 100644 > --- a/util/qemu-config.c > +++ b/util/qemu-config.c > @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname) > opts = qemu_opts_create(list, NULL, 0, &error_abort); > continue; > } > - if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) { > + if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 || > + (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) { This is one of the few times I've seen sscanf used in a well-defined manner (albeit still arbitrarily limiting, in that we have fixed-size buffers) - but having to rely on a comma operator to get there makes this look quite arcane. I still wonder if hand-rolling a real scanner would beat the compactness of sscanf by making the code intentions a little more discernible, and have the benefits of avoiding my sscanf red-flag checker. But my wonder is not enough to stop me from accepting this hack as-is. Reviewed-by: Eric Blake <eblake@redhat.com>
On 08/04/2015 20:16, Eduardo Habkost wrote: > Currently it is impossible to set an option in a config file to an empty > string, because the parser matches only lines containing non-empty > strings between double-quotes. > > As sscanf() "[" conversion specifier only matches non-empty strings, add > a special case for empty strings. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > util/qemu-config.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/util/qemu-config.c b/util/qemu-config.c > index 2d32ce7..9f9577d 100644 > --- a/util/qemu-config.c > +++ b/util/qemu-config.c > @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname) > opts = qemu_opts_create(list, NULL, 0, &error_abort); > continue; > } > - if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) { > + if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 || > + (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) { > /* arg = value */ > if (opts == NULL) { > error_report("no group defined"); > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
On Thu, Apr 09, 2015 at 02:11:11PM +0200, Paolo Bonzini wrote: > On 08/04/2015 20:16, Eduardo Habkost wrote: > > Currently it is impossible to set an option in a config file to an empty > > string, because the parser matches only lines containing non-empty > > strings between double-quotes. > > > > As sscanf() "[" conversion specifier only matches non-empty strings, add > > a special case for empty strings. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > util/qemu-config.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/util/qemu-config.c b/util/qemu-config.c > > index 2d32ce7..9f9577d 100644 > > --- a/util/qemu-config.c > > +++ b/util/qemu-config.c > > @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname) > > opts = qemu_opts_create(list, NULL, 0, &error_abort); > > continue; > > } > > - if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) { > > + if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 || > > + (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) { > > /* arg = value */ > > if (opts == NULL) { > > error_report("no group defined"); > > > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> Thanks! Whose tree this should go through?
On 09/04/2015 20:50, Eduardo Habkost wrote: > On Thu, Apr 09, 2015 at 02:11:11PM +0200, Paolo Bonzini wrote: >> On 08/04/2015 20:16, Eduardo Habkost wrote: >>> Currently it is impossible to set an option in a config file to an empty >>> string, because the parser matches only lines containing non-empty >>> strings between double-quotes. >>> >>> As sscanf() "[" conversion specifier only matches non-empty strings, add >>> a special case for empty strings. >>> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>> --- >>> util/qemu-config.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/util/qemu-config.c b/util/qemu-config.c >>> index 2d32ce7..9f9577d 100644 >>> --- a/util/qemu-config.c >>> +++ b/util/qemu-config.c >>> @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname) >>> opts = qemu_opts_create(list, NULL, 0, &error_abort); >>> continue; >>> } >>> - if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) { >>> + if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 || >>> + (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) { >>> /* arg = value */ >>> if (opts == NULL) { >>> error_report("no group defined"); >>> >> >> Acked-by: Paolo Bonzini <pbonzini@redhat.com> > > Thanks! Whose tree this should go through? Acked-by generally means that you're welcome to take it through yours. My "misc" tree hosts non-trivial patches outside well-defined maintainership area, but it's less work to carry patches only from people who don't send pull requests. Maybe it's not the case of this patch, but using your own tree also means that it's simpler for you to sort out the dependencies. Paolo
diff --git a/util/qemu-config.c b/util/qemu-config.c index 2d32ce7..9f9577d 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname) opts = qemu_opts_create(list, NULL, 0, &error_abort); continue; } - if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) { + if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 || + (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) { /* arg = value */ if (opts == NULL) { error_report("no group defined");
Currently it is impossible to set an option in a config file to an empty string, because the parser matches only lines containing non-empty strings between double-quotes. As sscanf() "[" conversion specifier only matches non-empty strings, add a special case for empty strings. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- util/qemu-config.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)