From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
To: Li Zhang <li.zhang@cloud.ionos.com>
Cc: Lukas Straub <lukasstraub2@web.de>,
alexandr.iarygin@profitbricks.com,
Markus Armbruster <armbru@redhat.com>,
QEMU <qemu-devel@nongnu.org>,
Pankaj Gupta <pankaj.gupta@cloud.ionos.com>,
Li Zhang <zhlcindy@gmail.com>
Subject: Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance
Date: Tue, 16 Mar 2021 19:24:54 +0400 [thread overview]
Message-ID: <CAJ+F1CJway2T9q-hHPhV1_37TLkUB=FV-5HA2+F4vm-kY=DdTA@mail.gmail.com> (raw)
In-Reply-To: <CAEM4iGGmwDToV04dcUczAVcVHAQREc-Th6e=qDsT4kp9ozM2XQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2955 bytes --]
Hi
On Tue, Mar 16, 2021 at 6:46 PM Li Zhang <li.zhang@cloud.ionos.com> wrote:
> Hi Marc-André,
>
> By looking into chardev and yank_register_function logic, this old
> chardev is registered according to the chardev label.
> So it's been in yank_instance_list. yank instance only has a chardev
> label, and the new chardev's label is the same as the old chardev.
> So it doesn't need to register it again when changing the chardev backend.
> Otherwise, it will report duplicated yank instances.
> I think the chardev logic has no problems. And it works with yank
> functions.
>
>
The previous instance is being removed with yank_unregister_instance()
during object_unparent(). The new instance is not registered.
That scenario deserves tests. (it's a shame there are no tests for yank ...)
> Thanks
> Li
>
> On Mon, Mar 15, 2021 at 7:51 PM Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Mon, Mar 15, 2021 at 9:22 PM Li Zhang <zhlcindy@gmail.com> wrote:
>>
>>> From: Li Zhang <li.zhang@cloud.ionos.com>
>>>
>>> When executing the QMP commands "chardev-change" to change the
>>> backend device to socket, it will cause a segment fault because
>>> it assumes chr->label as non-NULL in function yank_register_instance.
>>> The function qmp_chardev_change calls chardev_new, which label
>>> is NULL when creating a new chardev. The label will be passed to
>>> yank_register_instance which causes a segment fault. The callchain
>>> is as the following:
>>> chardev_new ->
>>> qemu_char_open ->
>>> cc->open ->
>>> qmp_chardev_open_socket ->
>>> yank_register_instance
>>>
>>> Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
>>> ---
>>> chardev/char-socket.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>>> index c8bced76b7..26d5172682 100644
>>> --- a/chardev/char-socket.c
>>> +++ b/chardev/char-socket.c
>>> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr,
>>> qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
>>> }
>>>
>>> - if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>>> errp)) {
>>> - return;
>>> + if (chr->label) {
>>> + if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label),
>>> errp)) {
>>> + return;
>>> + }
>>> + s->registered_yank = true;
>>> }
>>> - s->registered_yank = true;
>>>
>>> /* be isn't opened until we get a connection */
>>> *be_opened = false
>>>
>>
>> Looks wrong to me, the new chardev will get the same label, and it should
>> still be possible to call the yank functions then. The registration logic
>> needs to be reworked during chardev-change.
>>
>> --
>> Marc-André Lureau
>>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 4530 bytes --]
next prev parent reply other threads:[~2021-03-16 15:27 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-15 17:06 [PATCH 1/2] Fix the segment fault when calling yank_register_instance Li Zhang
2021-03-15 17:06 ` [PATCH 2/2] Support monitor chardev hotswap with QMP Li Zhang
2021-03-23 13:14 ` Li Zhang
2021-03-26 14:40 ` Markus Armbruster
2021-03-26 15:19 ` Li Zhang
2021-04-12 12:41 ` Li Zhang
2021-04-12 13:10 ` Markus Armbruster
2021-04-12 13:41 ` Li Zhang
2021-04-12 14:19 ` Pankaj Gupta
2021-04-13 6:40 ` Markus Armbruster
2021-04-13 8:51 ` Li Zhang
2021-04-16 9:33 ` Markus Armbruster
2021-04-16 9:52 ` Li Zhang
2021-04-13 8:57 ` Daniel P. Berrangé
2021-04-13 9:33 ` Li Zhang
2021-03-15 18:51 ` [PATCH 1/2] Fix the segment fault when calling yank_register_instance Marc-André Lureau
2021-03-16 9:45 ` Li Zhang
2021-03-16 14:46 ` Li Zhang
2021-03-16 15:24 ` Marc-André Lureau [this message]
2021-03-16 15:36 ` Li Zhang
2021-03-17 21:06 ` Lukas Straub
2021-03-26 14:41 ` Markus Armbruster
2021-03-26 16:02 ` Lukas Straub
2021-03-26 16:13 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJ+F1CJway2T9q-hHPhV1_37TLkUB=FV-5HA2+F4vm-kY=DdTA@mail.gmail.com' \
--to=marcandre.lureau@gmail.com \
--cc=alexandr.iarygin@profitbricks.com \
--cc=armbru@redhat.com \
--cc=li.zhang@cloud.ionos.com \
--cc=lukasstraub2@web.de \
--cc=pankaj.gupta@cloud.ionos.com \
--cc=qemu-devel@nongnu.org \
--cc=zhlcindy@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).