Message ID | 20220215055138.267904-1-rohit.kumar3@nutanix.com |
---|---|
State | New |
Headers | show |
Series | [v2] Check and report for incomplete 'global' option format | expand |
Rohit Kumar <rohit.kumar3@nutanix.com> writes: > Qemu might crash when provided incomplete '-global' option. > For example: > qemu-system-x86_64 -global driver=isa-fdc > qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394: > string_input_visitor_new: Assertion `str' failed. > Aborted (core dumped) > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604 The original qemu_global_option() only ever created QemuOpts with all three options present. Code consuming these QemuOpts relies on this invariant. Commit 3751d7c43f "vl: allow full-blown QemuOpts syntax for -global" (v2.4.0) wrecked it. Let's point to the root cause: Fixes: 3751d7c43f795b45ffdb9429cfb09c6beea55c68 > Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> > --- > diff to v1: > - Removed '\n' from error log message. > > softmmu/qdev-monitor.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index 01f3834db5..51b33caeca 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -1020,6 +1020,7 @@ int qemu_global_option(const char *str) > char driver[64], property[64]; > QemuOpts *opts; > int rc, offset; > + Error *err = NULL; > > rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, &offset); > if (rc == 2 && str[offset] == '=') { > @@ -1031,7 +1032,12 @@ int qemu_global_option(const char *str) > } > > opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false); > - if (!opts) { > + if (!opts || !qemu_opt_get(opts, "driver") || !qemu_opt_get(opts,"property") || > + !qemu_opt_get(opts, "value")) { > + error_setg(&err, "Invalid 'global' option format! " > + "Use -global <driver>.<property>=<value> or " > + "-global driver=driver,property=property,value=value"); > + error_report_err(err); > return -1; > } This fix isn't quite right. When qemu_opts_parse_noisily() fails, it reports an error and returns null. Your patch reports a second error then. Reproducer: $ qemu-system-x86_64 -global = qemu-system-x86_64: -global =: Invalid parameter '' qemu-system-x86_64: -global =: Invalid 'global' option format! Use -global <driver>.<property>=<value> or -global driver=driver,property=property,value=value You should do something like opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false); if (!opts) { return -1; } if (!qemu_opt_get(opts, "driver") || !qemu_opt_get(opts, "property") || !qemu_opt_get(opts, "value")) { error_report("options 'driver', 'property', and 'value'" " are required'); return -1; }
On 15/02/22 3:00 pm, Markus Armbruster wrote: > Rohit Kumar <rohit.kumar3@nutanix.com> writes: > >> Qemu might crash when provided incomplete '-global' option. >> For example: >> qemu-system-x86_64 -global driver=isa-fdc >> qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394: >> string_input_visitor_new: Assertion `str' failed. >> Aborted (core dumped) >> >> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_604&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs&m=YfuOt9ozXVjjleGOXTdWK4Hu8PoU3kTzGTXAJ223138_AM934NRnuqUQHpaLSWTs&s=WEGcm58m7pR4XrWGsUvHOVPj18ym0OrlAObwY_CfKlc&e= > The original qemu_global_option() only ever created QemuOpts with all > three options present. Code consuming these QemuOpts relies on this > invariant. Commit 3751d7c43f "vl: allow full-blown QemuOpts syntax for > -global" (v2.4.0) wrecked it. > > Let's point to the root cause: > > Fixes: 3751d7c43f795b45ffdb9429cfb09c6beea55c68 Thanks, Markus for pointing out this. I will add it in the next patch. > >> Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> >> --- >> diff to v1: >> - Removed '\n' from error log message. >> >> softmmu/qdev-monitor.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index 01f3834db5..51b33caeca 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -1020,6 +1020,7 @@ int qemu_global_option(const char *str) >> char driver[64], property[64]; >> QemuOpts *opts; >> int rc, offset; >> + Error *err = NULL; >> >> rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, &offset); >> if (rc == 2 && str[offset] == '=') { >> @@ -1031,7 +1032,12 @@ int qemu_global_option(const char *str) >> } >> >> opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false); >> - if (!opts) { >> + if (!opts || !qemu_opt_get(opts, "driver") || !qemu_opt_get(opts,"property") || >> + !qemu_opt_get(opts, "value")) { >> + error_setg(&err, "Invalid 'global' option format! " >> + "Use -global <driver>.<property>=<value> or " >> + "-global driver=driver,property=property,value=value"); >> + error_report_err(err); >> return -1; >> } > This fix isn't quite right. > > When qemu_opts_parse_noisily() fails, it reports an error and returns > null. Your patch reports a second error then. Reproducer: > > $ qemu-system-x86_64 -global = > qemu-system-x86_64: -global =: Invalid parameter '' > qemu-system-x86_64: -global =: Invalid 'global' option format! Use -global <driver>.<property>=<value> or -global driver=driver,property=property,value=value > > You should do something like > > opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false); > if (!opts) { > return -1; > } > if (!qemu_opt_get(opts, "driver") > || !qemu_opt_get(opts, "property") > || !qemu_opt_get(opts, "value")) { > error_report("options 'driver', 'property', and 'value'" > " are required'); > return -1; > } Ack., I will update it in the next patch. Thanks.
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 01f3834db5..51b33caeca 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -1020,6 +1020,7 @@ int qemu_global_option(const char *str) char driver[64], property[64]; QemuOpts *opts; int rc, offset; + Error *err = NULL; rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, &offset); if (rc == 2 && str[offset] == '=') { @@ -1031,7 +1032,12 @@ int qemu_global_option(const char *str) } opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false); - if (!opts) { + if (!opts || !qemu_opt_get(opts, "driver") || !qemu_opt_get(opts,"property") || + !qemu_opt_get(opts, "value")) { + error_setg(&err, "Invalid 'global' option format! " + "Use -global <driver>.<property>=<value> or " + "-global driver=driver,property=property,value=value"); + error_report_err(err); return -1; }
Qemu might crash when provided incomplete '-global' option. For example: qemu-system-x86_64 -global driver=isa-fdc qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394: string_input_visitor_new: Assertion `str' failed. Aborted (core dumped) Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604 Signed-off-by: Rohit Kumar <rohit.kumar3@nutanix.com> --- diff to v1: - Removed '\n' from error log message. softmmu/qdev-monitor.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)