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 <
[email protected]> wrote:
> Hi
>
> On Mon, Mar 15, 2021 at 9:22 PM Li Zhang <[email protected]> wrote:
>
>> From: Li Zhang <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>
