[Qemu-devel] [PATCH 0/2] fix segfault in (iscsi) option parsing

[Qemu-devel] [PATCH 0/2] fix segfault in (iscsi) option parsing

[Peter Lieven]

current git master segfaults if an iscsi option is specified
in command line.

Peter Lieven (2):
  qemu-option: avoid segfault if QemuOptsList == NULL
  vl.c: fix segfault in iscsi options parsing

 block/iscsi.c      |   27 ---------------------------
 util/qemu-option.c |    1 +
 vl.c               |   40 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 38 insertions(+), 30 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] qemu-option: avoid segfault if QemuOptsList == NULL

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 util/qemu-option.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 8b74bf1..24479d2 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -938,6 +938,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
     QemuOpts *opts;
     Error *local_err = NULL;
 
+    assert(list != NULL);
     assert(!permit_abbrev || list->implied_opt_name);
     firstname = permit_abbrev ? list->implied_opt_name : NULL;
 
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/2] vl.c: fix segfault in iscsi options parsing

[Peter Lieven]

this patch fixes parsing of iscsi options such as initiator-name
passed to command line via -iscsi option group.

because iscsi options where registered too late qemu_find_opts
returned NULL leading to a segfault in qemu_opts_parse.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |   27 ---------------------------
 vl.c          |   40 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 3d52921..23d4210 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1263,36 +1263,9 @@ static BlockDriver bdrv_iscsi = {
 #endif
 };
 
-static QemuOptsList qemu_iscsi_opts = {
-    .name = "iscsi",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
-    .desc = {
-        {
-            .name = "user",
-            .type = QEMU_OPT_STRING,
-            .help = "username for CHAP authentication to target",
-        },{
-            .name = "password",
-            .type = QEMU_OPT_STRING,
-            .help = "password for CHAP authentication to target",
-        },{
-            .name = "header-digest",
-            .type = QEMU_OPT_STRING,
-            .help = "HeaderDigest setting. "
-                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
-        },{
-            .name = "initiator-name",
-            .type = QEMU_OPT_STRING,
-            .help = "Initiator iqn name to use when connecting",
-        },
-        { /* end of list */ }
-    },
-};
-
 static void iscsi_block_init(void)
 {
     bdrv_register(&bdrv_iscsi);
-    qemu_add_opts(&qemu_iscsi_opts);
 }
 
 block_init(iscsi_block_init);
diff --git a/vl.c b/vl.c
index ce51e65..9925675 100644
--- a/vl.c
+++ b/vl.c
@@ -517,6 +517,34 @@ static QemuOptsList qemu_tpmdev_opts = {
     },
 };
 
+#ifdef CONFIG_LIBISCSI
+static QemuOptsList qemu_iscsi_opts = {
+    .name = "iscsi",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
+    .desc = {
+        {
+            .name = "user",
+            .type = QEMU_OPT_STRING,
+            .help = "username for CHAP authentication to target",
+        },{
+            .name = "password",
+            .type = QEMU_OPT_STRING,
+            .help = "password for CHAP authentication to target",
+        },{
+            .name = "header-digest",
+            .type = QEMU_OPT_STRING,
+            .help = "HeaderDigest setting. "
+                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
+        },{
+            .name = "initiator-name",
+            .type = QEMU_OPT_STRING,
+            .help = "Initiator iqn name to use when connecting",
+        },
+        { /* end of list */ }
+    },
+};
+#endif
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;
@@ -2899,6 +2927,9 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_add_fd_opts);
     qemu_add_opts(&qemu_object_opts);
     qemu_add_opts(&qemu_tpmdev_opts);
+#ifdef CONFIG_LIBISCSI
+    qemu_add_opts(&qemu_iscsi_opts);
+#endif
 
     runstate_init();
 
@@ -3199,14 +3230,17 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
-#ifdef CONFIG_LIBISCSI
             case QEMU_OPTION_iscsi:
-                opts = qemu_opts_parse(qemu_find_opts("iscsi"), optarg, 0);
+                olist = qemu_find_opts("iscsi");
+                if (!olist) {
+                    fprintf(stderr, "iscsi is not supported by this qemu build.\n");
+                    exit(1);
+                }
+                opts = qemu_opts_parse(olist, optarg, 0);
                 if (!opts) {
                     exit(1);
                 }
                 break;
-#endif
 #ifdef CONFIG_SLIRP
             case QEMU_OPTION_tftp:
                 legacy_tftp_prefix = optarg;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 0/2] fix segfault in (iscsi) option parsing

[Markus Armbruster]

Paolo, please have a look.

Peter Lieven <pl@kamp.de> writes:

> current git master segfaults if an iscsi option is specified
> in command line.
>
> Peter Lieven (2):
>   qemu-option: avoid segfault if QemuOptsList == NULL
>   vl.c: fix segfault in iscsi options parsing
>
>  block/iscsi.c      |   27 ---------------------------
>  util/qemu-option.c |    1 +
>  vl.c               |   40 +++++++++++++++++++++++++++++++++++++---
>  3 files changed, 38 insertions(+), 30 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] fix segfault in (iscsi) option parsing

[Paolo Bonzini]

Il 19/03/2013 09:51, Markus Armbruster ha scritto:
> Paolo, please have a look.

Why isn't it enough to call bdrv_init_with_whitelist earlier?

There is no conditional logic in it, the whitelist is checked at open time.

Paolo

> Peter Lieven <pl@kamp.de> writes:
> 
>> current git master segfaults if an iscsi option is specified
>> in command line.
>>
>> Peter Lieven (2):
>>   qemu-option: avoid segfault if QemuOptsList == NULL
>>   vl.c: fix segfault in iscsi options parsing
>>
>>  block/iscsi.c      |   27 ---------------------------
>>  util/qemu-option.c |    1 +
>>  vl.c               |   40 +++++++++++++++++++++++++++++++++++++---
>>  3 files changed, 38 insertions(+), 30 deletions(-)
> 
> 

Re: [Qemu-devel] [PATCH 0/2] fix segfault in (iscsi) option parsing

[Peter Lieven]

On 19.03.2013 12:18, Paolo Bonzini wrote:
> Il 19/03/2013 09:51, Markus Armbruster ha scritto:
>> Paolo, please have a look.
> Why isn't it enough to call bdrv_init_with_whitelist earlier?
>
> There is no conditional logic in it, the whitelist is checked at open time.

Has anyone tested if -spice is working? In this case bdrv_init_with_whitelist() won't help here.

Anyway, I would like to change the -iscsi code in vl.c to throw an error, if iscsi support is not
compiled in (like it is done with spice).

Peter

>
> Paolo
>
>> Peter Lieven <pl@kamp.de> writes:
>>
>>> current git master segfaults if an iscsi option is specified
>>> in command line.
>>>
>>> Peter Lieven (2):
>>>    qemu-option: avoid segfault if QemuOptsList == NULL
>>>    vl.c: fix segfault in iscsi options parsing
>>>
>>>   block/iscsi.c      |   27 ---------------------------
>>>   util/qemu-option.c |    1 +
>>>   vl.c               |   40 +++++++++++++++++++++++++++++++++++++---
>>>   3 files changed, 38 insertions(+), 30 deletions(-)
>>


-- 

Mit freundlichen Grüßen

Peter Lieven

...........................................................

   KAMP Netzwerkdienste GmbH
   Vestische Str. 89-91 | 46117 Oberhausen
   Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
   pl@kamp.de | http://www.kamp.de

   Geschäftsführer: Heiner Lante | Michael Lante
   Amtsgericht Duisburg | HRB Nr. 12154
   USt-Id-Nr.: DE 120607556

...........................................................

Re: [Qemu-devel] [PATCH 0/2] fix segfault in (iscsi) option parsing

[Paolo Bonzini]

Il 19/03/2013 16:54, Peter Lieven ha scritto:
> On 19.03.2013 12:18, Paolo Bonzini wrote:
>> Il 19/03/2013 09:51, Markus Armbruster ha scritto:
>>> Paolo, please have a look.
>> Why isn't it enough to call bdrv_init_with_whitelist earlier?
>>
>> There is no conditional logic in it, the whitelist is checked at open
>> time.
> 
> Has anyone tested if -spice is working?

It is.

machine_init entries are processed just below the other qemu_add_opts.

Paolo

 In this case
> bdrv_init_with_whitelist() won't help here.
> 
> Anyway, I would like to change the -iscsi code in vl.c to throw an
> error, if iscsi support is not
> compiled in (like it is done with spice).
> 
> Peter
> 
>>
>> Paolo
>>
>>> Peter Lieven <pl@kamp.de> writes:
>>>
>>>> current git master segfaults if an iscsi option is specified
>>>> in command line.
>>>>
>>>> Peter Lieven (2):
>>>>    qemu-option: avoid segfault if QemuOptsList == NULL
>>>>    vl.c: fix segfault in iscsi options parsing
>>>>
>>>>   block/iscsi.c      |   27 ---------------------------
>>>>   util/qemu-option.c |    1 +
>>>>   vl.c               |   40 +++++++++++++++++++++++++++++++++++++---
>>>>   3 files changed, 38 insertions(+), 30 deletions(-)
>>>
> 
> 

Re: [Qemu-devel] [PATCH 0/2] fix segfault in (iscsi) option parsing

[Peter Lieven]

Am 19.03.2013 um 18:07 schrieb Paolo Bonzini <pbonzini@redhat.com>:

> Il 19/03/2013 16:54, Peter Lieven ha scritto:
>> On 19.03.2013 12:18, Paolo Bonzini wrote:
>>> Il 19/03/2013 09:51, Markus Armbruster ha scritto:
>>>> Paolo, please have a look.
>>> Why isn't it enough to call bdrv_init_with_whitelist earlier?
>>> 
>>> There is no conditional logic in it, the whitelist is checked at open
>>> time.
>> 
>> Has anyone tested if -spice is working?
> 
> It is.
> 
> machine_init entries are processed just below the other qemu_add_opts.

Ok, if you agree I would prepare a new version of this patch set with 3 patches:

1) move up brd_init_with_whitelist
2) throw an error if -iscsi is used without compile libiscsi support
3) assert that QemuOptsList != NULL to avoid further segfaults.

I currently see a segfault on VM start in libiscsi with latest git. Whereas my old
qemu-kvm 1.2.0 is working with the same command line and libiscsi library.
As soon as I have sorted this out I will send a new patch set.

Peter


> 
> Paolo
> 
> In this case
>> bdrv_init_with_whitelist() won't help here.
>> 
>> Anyway, I would like to change the -iscsi code in vl.c to throw an
>> error, if iscsi support is not
>> compiled in (like it is done with spice).
>> 
>> Peter
>> 
>>> 
>>> Paolo
>>> 
>>>> Peter Lieven <pl@kamp.de> writes:
>>>> 
>>>>> current git master segfaults if an iscsi option is specified
>>>>> in command line.
>>>>> 
>>>>> Peter Lieven (2):
>>>>>   qemu-option: avoid segfault if QemuOptsList == NULL
>>>>>   vl.c: fix segfault in iscsi options parsing
>>>>> 
>>>>>  block/iscsi.c      |   27 ---------------------------
>>>>>  util/qemu-option.c |    1 +
>>>>>  vl.c               |   40 +++++++++++++++++++++++++++++++++++++---
>>>>>  3 files changed, 38 insertions(+), 30 deletions(-)
>>>> 
>> 
>> 
> 

Re: [Qemu-devel] [PATCH 0/2] fix segfault in (iscsi) option parsing

[Paolo Bonzini]

Il 19/03/2013 20:37, Peter Lieven ha scritto:
> 
> Am 19.03.2013 um 18:07 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
>> Il 19/03/2013 16:54, Peter Lieven ha scritto:
>>> On 19.03.2013 12:18, Paolo Bonzini wrote:
>>>> Il 19/03/2013 09:51, Markus Armbruster ha scritto:
>>>>> Paolo, please have a look.
>>>> Why isn't it enough to call bdrv_init_with_whitelist earlier?
>>>>
>>>> There is no conditional logic in it, the whitelist is checked at open
>>>> time.
>>>
>>> Has anyone tested if -spice is working?
>>
>> It is.
>>
>> machine_init entries are processed just below the other qemu_add_opts.
> 
> Ok, if you agree I would prepare a new version of this patch set with 3 patches:
> 
> 1) move up brd_init_with_whitelist
> 2) throw an error if -iscsi is used without compile libiscsi support
> 3) assert that QemuOptsList != NULL to avoid further segfaults.
> 
> I currently see a segfault on VM start in libiscsi with latest git. Whereas my old
> qemu-kvm 1.2.0 is working with the same command line and libiscsi library.
> As soon as I have sorted this out I will send a new patch set.

I don't think (3) is that important, but yes, that's the plan.

Paolo