diff mbox

[v2] qemu_opt_foreach: Fix crasher

Message ID 8828dfd6d813ab5e0b6c66a68e62b54ca2ada086.1475757380.git.mprivozn@redhat.com
State New
Headers show

Commit Message

Michal Prívozník Oct. 6, 2016, 12:39 p.m. UTC 
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650 <net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at util/qemu-option.c:617
617         QTAILQ_FOREACH(opt, &opts->head, next) {
[Current thread is 1 (Thread 0x7f1d4970bb40 (LWP 6603))]
(gdb) bt
#0  0x000055baf6ab4adc in qemu_opt_foreach (opts=0x0, func=0x55baf696b650 <net_vhost_chardev_opts>, opaque=0x7ffc51368c00, errp=0x7ffc51368e48) at util/qemu-option.c:617
#1  0x000055baf696b7da in net_vhost_parse_chardev (opts=0x55baf8ff9260, errp=0x7ffc51368e48) at net/vhost-user.c:314
#2  0x000055baf696b985 in net_init_vhost_user (netdev=0x55baf8ff9250, name=0x55baf879d270 "hostnet2", peer=0x0, errp=0x7ffc51368e48) at net/vhost-user.c:360
#3  0x000055baf6960216 in net_client_init1 (object=0x55baf8ff9250, is_netdev=true, errp=0x7ffc51368e48) at net/net.c:1051
#4  0x000055baf6960518 in net_client_init (opts=0x55baf776e7e0, is_netdev=true, errp=0x7ffc51368f00) at net/net.c:1108
#5  0x000055baf696083f in netdev_add (opts=0x55baf776e7e0, errp=0x7ffc51368f00) at net/net.c:1186
#6  0x000055baf69608c7 in qmp_netdev_add (qdict=0x55baf7afaf60, ret=0x7ffc51368f50, errp=0x7ffc51368f48) at net/net.c:1205
#7  0x000055baf6622135 in handle_qmp_command (parser=0x55baf77fb590, tokens=0x7f1d24011960) at /path/to/qemu.git/monitor.c:3978
#8  0x000055baf6a9d099 in json_message_process_token (lexer=0x55baf77fb598, input=0x55baf75acd20, type=JSON_RCURLY, x=113, y=19) at qobject/json-streamer.c:105
#9  0x000055baf6abf7aa in json_lexer_feed_char (lexer=0x55baf77fb598, ch=125 '}', flush=false) at qobject/json-lexer.c:319
#10 0x000055baf6abf8f2 in json_lexer_feed (lexer=0x55baf77fb598, buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-lexer.c:369
#11 0x000055baf6a9d13c in json_message_parser_feed (parser=0x55baf77fb590, buffer=0x7ffc51369170 "}R\204\367\272U", size=1) at qobject/json-streamer.c:124
#12 0x000055baf66221f7 in monitor_qmp_read (opaque=0x55baf77fb530, buf=0x7ffc51369170 "}R\204\367\272U", size=1) at /path/to/qemu.git/monitor.c:3994
#13 0x000055baf6757014 in qemu_chr_be_write_impl (s=0x55baf7610a40, buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:387
#14 0x000055baf6757076 in qemu_chr_be_write (s=0x55baf7610a40, buf=0x7ffc51369170 "}R\204\367\272U", len=1) at qemu-char.c:399
#15 0x000055baf675b3b0 in tcp_chr_read (chan=0x55baf90244b0, cond=G_IO_IN, opaque=0x55baf7610a40) at qemu-char.c:2927
#16 0x000055baf6a5d655 in qio_channel_fd_source_dispatch (source=0x55baf7610df0, callback=0x55baf675b25a <tcp_chr_read>, user_data=0x55baf7610a40) at io/channel-watch.c:84
#17 0x00007f1d3e80cbbd in g_main_context_dispatch () from /usr/lib64/libglib-2.0.so.0
#18 0x000055baf69d3720 in glib_pollfds_poll () at main-loop.c:213
#19 0x000055baf69d37fd in os_host_main_loop_wait (timeout=126000000) at main-loop.c:258
#20 0x000055baf69d38ad in main_loop_wait (nonblocking=0) at main-loop.c:506
#21 0x000055baf676587b in main_loop () at vl.c:1908
#22 0x000055baf676d3bf in main (argc=101, argv=0x7ffc5136a6c8, envp=0x7ffc5136a9f8) at vl.c:4604
(gdb) p opts
$1 = (QemuOpts *) 0x0

The crash occurred when I tried to attach vhost-user net:

{"execute":"chardev-add","arguments":{"id":"charnet2","backend":{"type":"socket","data":{"addr":{"type":"unix","data":{"path":"/var/run/openvswitch/vhost-user1"}},"wait":false,"server":false}}},"id":"libvirt-19"}
{"return": {}, "id": "libvirt-19"}
{"execute":"netdev_add","arguments":{"type":"vhost-user","chardev":"charnet2","id":"hostnet2"},"id":"libvirt-20"}

2016-08-16 14:52:18.274+0000: 6456: error : qemuMonitorIORead:586 : Unable to read from monitor: Connection reset by peer


The solution is to teach qemu_opt_foreach() to take a shortcut if
@opts is NULL.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

v2 of:

http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02933.html

Comments

Peter Maydell Oct. 6, 2016, 1:02 p.m. UTC | #1 
On 6 October 2016 at 13:39, Michal Privoznik <mprivozn@redhat.com> wrote:
> The crash occurred when I tried to attach vhost-user net:
>
> {"execute":"chardev-add","arguments":{"id":"charnet2","backend":{"type":"socket","data":{"addr":{"type":"unix","data":{"path":"/var/run/openvswitch/vhost-user1"}},"wait":false,"server":false}}},"id":"libvirt-19"}
> {"return": {}, "id": "libvirt-19"}
> {"execute":"netdev_add","arguments":{"type":"vhost-user","chardev":"charnet2","id":"hostnet2"},"id":"libvirt-20"}
>
> 2016-08-16 14:52:18.274+0000: 6456: error : qemuMonitorIORead:586 : Unable to read from monitor: Connection reset by peer
>
>
> The solution is to teach qemu_opt_foreach() to take a shortcut if
> @opts is NULL.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>
> v2 of:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02933.html
>
> diff to v1:
> - Added comment to CharDriverState struct to warn others too.
>
>  include/sysemu/char.h | 1 +
>  util/qemu-option.c    | 5 +++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 0d0465a..df58ef8 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -93,6 +93,7 @@ struct CharDriverState {
>      int avail_connections;
>      int is_mux;
>      guint fd_in_tag;
> +    /* Be aware that in some cases @opts might be NULL. */
>      QemuOpts *opts;
>      bool replay;
>      QTAILQ_ENTRY(CharDriverState) next;

Wouldn't it be better to ensure that it can't be NULL regardless of
how the object was created?

thanks
-- PMM
Peter Maydell Oct. 6, 2016, 1:10 p.m. UTC | #2 
On 6 October 2016 at 14:02, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 October 2016 at 13:39, Michal Privoznik <mprivozn@redhat.com> wrote:
>> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>> index 0d0465a..df58ef8 100644
>> --- a/include/sysemu/char.h
>> +++ b/include/sysemu/char.h
>> @@ -93,6 +93,7 @@ struct CharDriverState {
>>      int avail_connections;
>>      int is_mux;
>>      guint fd_in_tag;
>> +    /* Be aware that in some cases @opts might be NULL. */
>>      QemuOpts *opts;
>>      bool replay;
>>      QTAILQ_ENTRY(CharDriverState) next;
>
> Wouldn't it be better to ensure that it can't be NULL regardless of
> how the object was created?

FWIW, a quick check of who uses this field (by commenting it out
and looking for the compile errors) shows:
 net/vhost-user.c
 net/colo-compare.c (which also just does a foreach on it)
 qemu-char.c (in qemu_chr_new_from_opts and qemu_chr_free_common)

Alternative possible approach: if you can create a CharDriver
validly with no opts, then the net/ code shouldn't be looking
at opts at all (but should instead look at wherever the config
stuff goes for the 'opts is null' case, which we should make
sure is correct regardless of how the CharDriver was created).

thanks
-- PMM
Daniel P. Berrangé Oct. 6, 2016, 1:41 p.m. UTC | #3 
On Thu, Oct 06, 2016 at 02:10:17PM +0100, Peter Maydell wrote:
> On 6 October 2016 at 14:02, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 6 October 2016 at 13:39, Michal Privoznik <mprivozn@redhat.com> wrote:
> >> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> >> index 0d0465a..df58ef8 100644
> >> --- a/include/sysemu/char.h
> >> +++ b/include/sysemu/char.h
> >> @@ -93,6 +93,7 @@ struct CharDriverState {
> >>      int avail_connections;
> >>      int is_mux;
> >>      guint fd_in_tag;
> >> +    /* Be aware that in some cases @opts might be NULL. */
> >>      QemuOpts *opts;
> >>      bool replay;
> >>      QTAILQ_ENTRY(CharDriverState) next;
> >
> > Wouldn't it be better to ensure that it can't be NULL regardless of
> > how the object was created?
> 
> FWIW, a quick check of who uses this field (by commenting it out
> and looking for the compile errors) shows:
>  net/vhost-user.c
>  net/colo-compare.c (which also just does a foreach on it)
>  qemu-char.c (in qemu_chr_new_from_opts and qemu_chr_free_common)
> 
> Alternative possible approach: if you can create a CharDriver
> validly with no opts, then the net/ code shouldn't be looking
> at opts at all (but should instead look at wherever the config
> stuff goes for the 'opts is null' case, which we should make
> sure is correct regardless of how the CharDriver was created).

Agreed, that net/vhost-user.c code is just evil - it has no business
poking around the chardev internals in that way. AFAICT, the only
reason it is doing that is so that it can report an error if you
try to connect the vhost-suer to a chardev that isn't  socket
backed.

If it requires some particular feature of the chardev backend, then
we should make a way to query for existance of that feature directly.

Regards,
Daniel
Markus Armbruster Oct. 6, 2016, 2:51 p.m. UTC | #4 
"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Thu, Oct 06, 2016 at 02:10:17PM +0100, Peter Maydell wrote:
>> On 6 October 2016 at 14:02, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > On 6 October 2016 at 13:39, Michal Privoznik <mprivozn@redhat.com> wrote:
>> >> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>> >> index 0d0465a..df58ef8 100644
>> >> --- a/include/sysemu/char.h
>> >> +++ b/include/sysemu/char.h
>> >> @@ -93,6 +93,7 @@ struct CharDriverState {
>> >>      int avail_connections;
>> >>      int is_mux;
>> >>      guint fd_in_tag;
>> >> +    /* Be aware that in some cases @opts might be NULL. */
>> >>      QemuOpts *opts;
>> >>      bool replay;
>> >>      QTAILQ_ENTRY(CharDriverState) next;
>> >
>> > Wouldn't it be better to ensure that it can't be NULL regardless of
>> > how the object was created?
>> 
>> FWIW, a quick check of who uses this field (by commenting it out
>> and looking for the compile errors) shows:
>>  net/vhost-user.c
>>  net/colo-compare.c (which also just does a foreach on it)
>>  qemu-char.c (in qemu_chr_new_from_opts and qemu_chr_free_common)
>> 
>> Alternative possible approach: if you can create a CharDriver
>> validly with no opts, then the net/ code shouldn't be looking
>> at opts at all (but should instead look at wherever the config
>> stuff goes for the 'opts is null' case, which we should make
>> sure is correct regardless of how the CharDriver was created).
>
> Agreed, that net/vhost-user.c code is just evil - it has no business
> poking around the chardev internals in that way. AFAICT, the only
> reason it is doing that is so that it can report an error if you
> try to connect the vhost-suer to a chardev that isn't  socket
> backed.
>
> If it requires some particular feature of the chardev backend, then
> we should make a way to query for existance of that feature directly.

Seconded.
diff mbox

Patch

diff to v1:
- Added comment to CharDriverState struct to warn others too.

 include/sysemu/char.h | 1 +
 util/qemu-option.c    | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 0d0465a..df58ef8 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -93,6 +93,7 @@  struct CharDriverState {
     int avail_connections;
     int is_mux;
     guint fd_in_tag;
+    /* Be aware that in some cases @opts might be NULL. */
     QemuOpts *opts;
     bool replay;
     QTAILQ_ENTRY(CharDriverState) next;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 3467dc2..78be7e1 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -614,6 +614,11 @@  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
     QemuOpt *opt;
     int rc;
 
+    if (!opts) {
+        /* Done, trivially. */
+        return 0;
+    }
+
     QTAILQ_FOREACH(opt, &opts->head, next) {
         rc = func(opaque, opt->name, opt->str, errp);
         if (rc) {