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

[Li Zhang <li.zhang@cloud.ionos.com>]

[-- Attachment #1: Type: text/plain, Size: 2924 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 4161 bytes --]