Hi On Tue, Mar 16, 2021 at 6:46 PM Li Zhang <[email protected]> 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 < > [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 >> > -- Marc-André Lureau
