| Message ID | 1385658779-7529-1-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
Laszlo Ersek <lersek@redhat.com> writes: > qemu_opts_parse() can always return NULL, even if the QemuOptsList.desc in > question would be trivial to satisfy (eg. because it's empty). For > example: > > qemu_opts_parse() > opts_parse() > qemu_opts_create() > id_wellformed() > > In practice: > > $ .../qemu-system-x86_64 -acpitable id=3 > qemu-system-x86_64: -acpitable id=3: Parameter 'id' expects an identifier > ** > ERROR:vl.c:3491:main: assertion failed: (opts != NULL) > Aborted (core dumped) > > $ .../qemu-system-x86_64 -smbios id=3 > qemu-system-x86_64: -smbios id=3: Parameter 'id' expects an identifier > Segmentation fault (core dumped) > > I checked all qemu_opts_parse() invocations (and all drive_def() > invocations too, because it blindly forwards the former's retval). Only > the two above examples look problematic. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> Your fix is fine. I didn't redo your search for other unsafe uses. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 11/28/13 19:32, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> writes: > >> qemu_opts_parse() can always return NULL, even if the QemuOptsList.desc in >> question would be trivial to satisfy (eg. because it's empty). For >> example: >> >> qemu_opts_parse() >> opts_parse() >> qemu_opts_create() >> id_wellformed() >> >> In practice: >> >> $ .../qemu-system-x86_64 -acpitable id=3 >> qemu-system-x86_64: -acpitable id=3: Parameter 'id' expects an identifier >> ** >> ERROR:vl.c:3491:main: assertion failed: (opts != NULL) >> Aborted (core dumped) >> >> $ .../qemu-system-x86_64 -smbios id=3 >> qemu-system-x86_64: -smbios id=3: Parameter 'id' expects an identifier >> Segmentation fault (core dumped) >> >> I checked all qemu_opts_parse() invocations (and all drive_def() >> invocations too, because it blindly forwards the former's retval). Only >> the two above examples look problematic. >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > Your fix is fine. I didn't redo your search for other unsafe uses. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Ping -- please commit this. Thanks Laszlo
On Thu, Nov 28, 2013 at 06:12:59PM +0100, Laszlo Ersek wrote: > qemu_opts_parse() can always return NULL, even if the QemuOptsList.desc in > question would be trivial to satisfy (eg. because it's empty). For > example: > > qemu_opts_parse() > opts_parse() > qemu_opts_create() > id_wellformed() > > In practice: > > $ .../qemu-system-x86_64 -acpitable id=3 > qemu-system-x86_64: -acpitable id=3: Parameter 'id' expects an identifier > ** > ERROR:vl.c:3491:main: assertion failed: (opts != NULL) > Aborted (core dumped) > > $ .../qemu-system-x86_64 -smbios id=3 > qemu-system-x86_64: -smbios id=3: Parameter 'id' expects an identifier > Segmentation fault (core dumped) > > I checked all qemu_opts_parse() invocations (and all drive_def() > invocations too, because it blindly forwards the former's retval). Only > the two above examples look problematic. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> Applied, thanks. > --- > vl.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/vl.c b/vl.c > index 8d5d874..1863fdd 100644 > --- a/vl.c > +++ b/vl.c > @@ -3488,11 +3488,16 @@ int main(int argc, char **argv, char **envp) > } > case QEMU_OPTION_acpitable: > opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1); > - g_assert(opts != NULL); > + if (!opts) { > + exit(1); > + } > do_acpitable_option(opts); > break; > case QEMU_OPTION_smbios: > opts = qemu_opts_parse(qemu_find_opts("smbios"), optarg, 0); > + if (!opts) { > + exit(1); > + } > do_smbios_option(opts); > break; > case QEMU_OPTION_enable_kvm: > -- > 1.8.3.1 >
diff --git a/vl.c b/vl.c index 8d5d874..1863fdd 100644 --- a/vl.c +++ b/vl.c @@ -3488,11 +3488,16 @@ int main(int argc, char **argv, char **envp) } case QEMU_OPTION_acpitable: opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1); - g_assert(opts != NULL); + if (!opts) { + exit(1); + } do_acpitable_option(opts); break; case QEMU_OPTION_smbios: opts = qemu_opts_parse(qemu_find_opts("smbios"), optarg, 0); + if (!opts) { + exit(1); + } do_smbios_option(opts); break; case QEMU_OPTION_enable_kvm:
qemu_opts_parse() can always return NULL, even if the QemuOptsList.desc in question would be trivial to satisfy (eg. because it's empty). For example: qemu_opts_parse() opts_parse() qemu_opts_create() id_wellformed() In practice: $ .../qemu-system-x86_64 -acpitable id=3 qemu-system-x86_64: -acpitable id=3: Parameter 'id' expects an identifier ** ERROR:vl.c:3491:main: assertion failed: (opts != NULL) Aborted (core dumped) $ .../qemu-system-x86_64 -smbios id=3 qemu-system-x86_64: -smbios id=3: Parameter 'id' expects an identifier Segmentation fault (core dumped) I checked all qemu_opts_parse() invocations (and all drive_def() invocations too, because it blindly forwards the former's retval). Only the two above examples look problematic. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- vl.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)