diff mbox series

[1/2] Fix the segment fault when calling yank_register_instance

Message ID 20210315170636.704201-1-zhlcindy@gmail.com
State New
Headers show
Series [1/2] Fix the segment fault when calling yank_register_instance | expand

Commit Message

Li Zhang March 15, 2021, 5:06 p.m. UTC 
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(-)

Comments

Marc-André Lureau March 15, 2021, 6:51 p.m. UTC | #1 
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.
Li Zhang March 16, 2021, 9:45 a.m. UTC | #2 
Hi Marc-André,

The new chardev can get the same label. It is assigned after the function

ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
                                  Error **errp)
{
     .....
     chr_new = chardev_new(NULL, object_class_get_name(OBJECT_CLASS(cc)),
                          backend, chr->gcontext, errp);
    if (!chr_new) {
        return NULL;
    }
    chr_new->label = g_strdup(id);
    if (chr->be_open && !chr_new->be_open) {
        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
        closed_sent = true;
    }

    chr->be = NULL;
    qemu_chr_fe_init(be, chr_new, &error_abort);
       .....
}

It passes parameter NULL in chardev_new, I think it may be because the old
chardev isn't released yet.
It will cause duplicated problems. I need to consider this logic to see if
it can be changed.

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
>
Li Zhang March 16, 2021, 2:46 p.m. UTC | #3 
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.

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 March 16, 2021, 3:24 p.m. UTC | #4 
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
>>
>
Li Zhang March 16, 2021, 3:36 p.m. UTC | #5 
Hi Marc-André,

Hi Marc-André,

Ah, you are right.  For some scenarios, it is not registered and registered
for some scenarios.
If the previous chardev is not socket, it won't be registered either.
There are still problems.

On Tue, Mar 16, 2021 at 4:25 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> 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
>
Lukas Straub March 17, 2021, 9:06 p.m. UTC | #6 
On Mon, 15 Mar 2021 18:06:35 +0100
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

Oh, I didn't consider the chardev-change case. I'll look into it.

Regards,
Lukas Straub

--
Markus Armbruster March 26, 2021, 2:41 p.m. UTC | #7 
Looks like a bug fix.  Lukas, can you take care of it in time for 6.0?

Li Zhang <zhlcindy@gmail.com> writes:

> 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;
Lukas Straub March 26, 2021, 4:02 p.m. UTC | #8 
On Fri, 26 Mar 2021 15:41:11 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Looks like a bug fix.  Lukas, can you take care of it in time for 6.0?
> 

Yeah, this patch only fixes a symptom, but not the core cause of the bug.
I have already written patches that fix the bugs inclusive test-cases:
https://lore.kernel.org/qemu-devel/cover.1616744509.git.lukasstraub2@web.de/

Regards,
Lukas Straub

--
Markus Armbruster March 26, 2021, 4:13 p.m. UTC | #9 
Lukas Straub <lukasstraub2@web.de> writes:

> On Fri, 26 Mar 2021 15:41:11 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Looks like a bug fix.  Lukas, can you take care of it in time for 6.0?
>> 
>
> Yeah, this patch only fixes a symptom, but not the core cause of the bug.
> I have already written patches that fix the bugs inclusive test-cases:
> https://lore.kernel.org/qemu-devel/cover.1616744509.git.lukasstraub2@web.de/

Awesome!  I didn't make the connection.  I trust you'll take care of
getting the fixes merged :)
diff mbox series

Patch

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;