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