[PATCH 0/2] vbox: Two simple fixes
I've notice these while working on adding support for 7.0 API. Michal Prívozník (2): vbox: Fix memleak in _virtualboxCreateMachine() vbox: Fix starting domains src/vbox/vbox_common.c | 13 ++++++------- src/vbox/vbox_tmpl.c | 1 + 2 files changed, 7 insertions(+), 7 deletions(-) -- 2.39.1
The _virtualboxCreateMachine() function allocates @createFlagsUtf16 but never frees it. ==12481== 236 bytes in 2 blocks are definitely lost in loss record 2,060 of 2,216 ==12481== at 0x48407E5: malloc (vg_replace_malloc.c:393) ==12481== by 0xB6C6D1B: RTStrToUtf16Tag (utf-8.cpp:1033) ==12481== by 0xB4DB500: _virtualboxCreateMachine (vbox_tmpl.c:634) ==12481== by 0xB4E68A3: vboxDomainDefineXMLFlags (vbox_common.c:1976) ==12481== by 0x4C7DF83: virDomainDefineXMLFlags (libvirt-domain.c:6666) ==12481== by 0x13C2DA: remoteDispatchDomainDefineXMLFlags (remote_daemon_dispatch_stubs.h:5271) ==12481== by 0x13C265: remoteDispatchDomainDefineXMLFlagsHelper (remote_daemon_dispatch_stubs.h:5252) ==12481== by 0x4AD9DF7: virNetServerProgramDispatchCall (virnetserverprogram.c:428) ==12481== by 0x4AD9931: virNetServerProgramDispatch (virnetserverprogram.c:302) ==12481== by 0x4AE28AC: virNetServerProcessMsg (virnetserver.c:135) ==12481== by 0x4AE2972: virNetServerHandleJob (virnetserver.c:155) ==12481== by 0x49BC275: virThreadPoolWorker (virthreadpool.c:164) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_tmpl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 52b1c93b6d..91609741e3 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -626,6 +626,7 @@ _virtualboxCreateMachine(struct _vboxDriver *data, virDomainDef *def, IMachine * machine); VIR_FREE(createFlags); VBOX_UTF16_FREE(machineNameUtf16); + VBOX_UTF16_FREE(createFlagsUtf16); vboxIIDUnalloc(&iid); return rc; } -- 2.39.1
On Mon, Jan 23, 2023 at 10:31:52AM +0100, Michal Privoznik wrote:
The _virtualboxCreateMachine() function allocates @createFlagsUtf16 but never frees it.
==12481== 236 bytes in 2 blocks are definitely lost in loss record 2,060 of 2,216 ==12481== at 0x48407E5: malloc (vg_replace_malloc.c:393) ==12481== by 0xB6C6D1B: RTStrToUtf16Tag (utf-8.cpp:1033) ==12481== by 0xB4DB500: _virtualboxCreateMachine (vbox_tmpl.c:634) ==12481== by 0xB4E68A3: vboxDomainDefineXMLFlags (vbox_common.c:1976) ==12481== by 0x4C7DF83: virDomainDefineXMLFlags (libvirt-domain.c:6666) ==12481== by 0x13C2DA: remoteDispatchDomainDefineXMLFlags (remote_daemon_dispatch_stubs.h:5271) ==12481== by 0x13C265: remoteDispatchDomainDefineXMLFlagsHelper (remote_daemon_dispatch_stubs.h:5252) ==12481== by 0x4AD9DF7: virNetServerProgramDispatchCall (virnetserverprogram.c:428) ==12481== by 0x4AD9931: virNetServerProgramDispatch (virnetserverprogram.c:302) ==12481== by 0x4AE28AC: virNetServerProcessMsg (virnetserver.c:135) ==12481== by 0x4AE2972: virNetServerHandleJob (virnetserver.c:155) ==12481== by 0x49BC275: virThreadPoolWorker (virthreadpool.c:164)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--- src/vbox/vbox_tmpl.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 52b1c93b6d..91609741e3 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -626,6 +626,7 @@ _virtualboxCreateMachine(struct _vboxDriver *data, virDomainDef *def, IMachine * machine); VIR_FREE(createFlags); VBOX_UTF16_FREE(machineNameUtf16); + VBOX_UTF16_FREE(createFlagsUtf16); vboxIIDUnalloc(&iid); return rc; } -- 2.39.1
When starting a VirtualBox domain, we try to guess which frontend to use. While the whole algorithm looks a bit outdated, it may happen that we tell VirtualBox to use "gui" frontend, but not which DISPLAY= to use. I haven't found any documentation on the algorithm we use, but if I make us fallback onto DISPLAY=:0 when no other configuration is found then I'm able to start my guests just fine. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bd77641d39..5269f9b23f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -2121,13 +2121,12 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine, vboxIID *iid VBOX_UTF8_FREE(valueDisplayUtf8); if (guiPresent) { - if (guiDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(guiDisplay); - } + char *displayutf8; + + displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay ? guiDisplay : ":0"); + VBOX_UTF8_TO_UTF16(displayutf8, &env); + VIR_FREE(displayutf8); + VIR_FREE(guiDisplay); VBOX_UTF8_TO_UTF16("gui", &sessionType); } -- 2.39.1
On Mon, Jan 23, 2023 at 10:31:53AM +0100, Michal Privoznik wrote:
When starting a VirtualBox domain, we try to guess which frontend to use. While the whole algorithm looks a bit outdated, it may happen that we tell VirtualBox to use "gui" frontend, but not which DISPLAY= to use.
I haven't found any documentation on the algorithm we use, but if I make us fallback onto DISPLAY=:0 when no other configuration is found then I'm able to start my guests just fine.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bd77641d39..5269f9b23f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -2121,13 +2121,12 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine, vboxIID *iid VBOX_UTF8_FREE(valueDisplayUtf8);
if (guiPresent) { - if (guiDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(guiDisplay); - } + char *displayutf8; + + displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay ? guiDisplay : ":0"); + VBOX_UTF8_TO_UTF16(displayutf8, &env);
This might get overwritten when using SDL couple lines below. I suggest you default to :0 only if none other option remains. It feels dirty just guessing the display number, but let's say that's something we'll have to live with in the vbox driver.
+ VIR_FREE(displayutf8); + VIR_FREE(guiDisplay);
VBOX_UTF8_TO_UTF16("gui", &sessionType); } -- 2.39.1
On 1/23/23 10:45, Martin Kletzander wrote:
On Mon, Jan 23, 2023 at 10:31:53AM +0100, Michal Privoznik wrote:
When starting a VirtualBox domain, we try to guess which frontend to use. While the whole algorithm looks a bit outdated, it may happen that we tell VirtualBox to use "gui" frontend, but not which DISPLAY= to use.
I haven't found any documentation on the algorithm we use, but if I make us fallback onto DISPLAY=:0 when no other configuration is found then I'm able to start my guests just fine.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bd77641d39..5269f9b23f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -2121,13 +2121,12 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine, vboxIID *iid VBOX_UTF8_FREE(valueDisplayUtf8);
if (guiPresent) { - if (guiDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(guiDisplay); - } + char *displayutf8; + + displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay ? guiDisplay : ":0"); + VBOX_UTF8_TO_UTF16(displayutf8, &env);
This might get overwritten when using SDL couple lines below. I suggest you default to :0 only if none other option remains. It feels dirty just guessing the display number, but let's say that's something we'll have to live with in the vbox driver.
I'm failing to see how guiPresent and sdlPresent can be set at the same time. But I guess I can join those two cases together, sure. Michal
On Mon, Jan 23, 2023 at 02:35:40PM +0100, Michal Prívozník wrote:
On 1/23/23 10:45, Martin Kletzander wrote:
On Mon, Jan 23, 2023 at 10:31:53AM +0100, Michal Privoznik wrote:
When starting a VirtualBox domain, we try to guess which frontend to use. While the whole algorithm looks a bit outdated, it may happen that we tell VirtualBox to use "gui" frontend, but not which DISPLAY= to use.
I haven't found any documentation on the algorithm we use, but if I make us fallback onto DISPLAY=:0 when no other configuration is found then I'm able to start my guests just fine.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bd77641d39..5269f9b23f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -2121,13 +2121,12 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine, vboxIID *iid VBOX_UTF8_FREE(valueDisplayUtf8);
if (guiPresent) { - if (guiDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(guiDisplay); - } + char *displayutf8; + + displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay ? guiDisplay : ":0"); + VBOX_UTF8_TO_UTF16(displayutf8, &env);
This might get overwritten when using SDL couple lines below. I suggest you default to :0 only if none other option remains. It feels dirty just guessing the display number, but let's say that's something we'll have to live with in the vbox driver.
I'm failing to see how guiPresent and sdlPresent can be set at the same time. But I guess I can join those two cases together, sure.
Sorry, I missed that, I'm probably too cautious, although there are some other places where we're trying to be equally cautious, e.g. vboxAttachDisplay: https://gitlab.com/libvirt/libvirt/-/blob/master/src/vbox/vbox_common.c#L165... But in any case, doesn't the block under sdlDisplay conditional need the same treatment?
Michal
On 1/23/23 15:22, Martin Kletzander wrote:
On Mon, Jan 23, 2023 at 02:35:40PM +0100, Michal Prívozník wrote:
On 1/23/23 10:45, Martin Kletzander wrote:
On Mon, Jan 23, 2023 at 10:31:53AM +0100, Michal Privoznik wrote:
When starting a VirtualBox domain, we try to guess which frontend to use. While the whole algorithm looks a bit outdated, it may happen that we tell VirtualBox to use "gui" frontend, but not which DISPLAY= to use.
I haven't found any documentation on the algorithm we use, but if I make us fallback onto DISPLAY=:0 when no other configuration is found then I'm able to start my guests just fine.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bd77641d39..5269f9b23f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -2121,13 +2121,12 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine, vboxIID *iid VBOX_UTF8_FREE(valueDisplayUtf8);
if (guiPresent) { - if (guiDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(guiDisplay); - } + char *displayutf8; + + displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay ? guiDisplay : ":0"); + VBOX_UTF8_TO_UTF16(displayutf8, &env);
This might get overwritten when using SDL couple lines below. I suggest you default to :0 only if none other option remains. It feels dirty just guessing the display number, but let's say that's something we'll have to live with in the vbox driver.
I'm failing to see how guiPresent and sdlPresent can be set at the same time. But I guess I can join those two cases together, sure.
Sorry, I missed that, I'm probably too cautious, although there are some other places where we're trying to be equally cautious, e.g. vboxAttachDisplay:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/vbox/vbox_common.c#L165...
But in any case, doesn't the block under sdlDisplay conditional need the same treatment?
Yeah, that's why I've sent v2: https://listman.redhat.com/archives/libvir-list/2023-January/237296.html Michal
When starting a VirtualBox domain, we try to guess which frontend to use. While the whole algorithm looks a bit outdated, it may happen that we tell VirtualBox to use "gui" frontend, but not which DISPLAY= to use. I haven't found any documentation on the algorithm we use, but if I make us fallback onto DISPLAY=:0 when no other configuration is found then I'm able to start my guests just fine. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- I've experimented with introducing an enum for frontend since it can be one of gui, sdl, vrdp. But it turned out to be a huge mess and not worth the fix I'm trying to get in. If anybody else wants to do it, please. src/vbox/vbox_common.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bd77641d39..30a85ce8b2 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -2120,32 +2120,33 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine, vboxIID *iid } VBOX_UTF8_FREE(valueDisplayUtf8); - if (guiPresent) { - if (guiDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(guiDisplay); - } + if (guiPresent || sdlPresent) { + const char *display = NULL; + const char *sessType = NULL; + char *displayutf8; - VBOX_UTF8_TO_UTF16("gui", &sessionType); - } + if (guiPresent) { + sessType = "gui"; + display = guiDisplay; + } else { + sessType = "sdl"; + display = sdlDisplay; + } - if (sdlPresent) { - if (sdlDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", sdlDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(sdlDisplay); + if (!display) { + /* Provide some sane default */ + display = ":0"; } - VBOX_UTF8_TO_UTF16("sdl", &sessionType); - } + displayutf8 = g_strdup_printf("DISPLAY=%s", display); + VBOX_UTF8_TO_UTF16(displayutf8, &env); + VIR_FREE(displayutf8); + VIR_FREE(guiDisplay); - if (vrdpPresent) + VBOX_UTF8_TO_UTF16(sessType, &sessionType); + } else if (vrdpPresent) { VBOX_UTF8_TO_UTF16("vrdp", &sessionType); + } rc = gVBoxAPI.UIMachine.LaunchVMProcess(data, machine, iid, sessionType, env, -- 2.39.1
On Mon, Jan 23, 2023 at 02:59:28PM +0100, Michal Privoznik wrote:
When starting a VirtualBox domain, we try to guess which frontend to use. While the whole algorithm looks a bit outdated, it may happen that we tell VirtualBox to use "gui" frontend, but not which DISPLAY= to use.
I haven't found any documentation on the algorithm we use, but if I make us fallback onto DISPLAY=:0 when no other configuration is found then I'm able to start my guests just fine.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
---
I've experimented with introducing an enum for frontend since it can be one of gui, sdl, vrdp. But it turned out to be a huge mess and not worth the fix I'm trying to get in. If anybody else wants to do it, please.
src/vbox/vbox_common.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index bd77641d39..30a85ce8b2 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -2120,32 +2120,33 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine, vboxIID *iid } VBOX_UTF8_FREE(valueDisplayUtf8);
- if (guiPresent) { - if (guiDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", guiDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(guiDisplay); - } + if (guiPresent || sdlPresent) { + const char *display = NULL; + const char *sessType = NULL; + char *displayutf8;
- VBOX_UTF8_TO_UTF16("gui", &sessionType); - } + if (guiPresent) { + sessType = "gui"; + display = guiDisplay; + } else { + sessType = "sdl"; + display = sdlDisplay; + }
- if (sdlPresent) { - if (sdlDisplay) { - char *displayutf8; - displayutf8 = g_strdup_printf("DISPLAY=%s", sdlDisplay); - VBOX_UTF8_TO_UTF16(displayutf8, &env); - VIR_FREE(displayutf8); - VIR_FREE(sdlDisplay); + if (!display) { + /* Provide some sane default */ + display = ":0"; }
- VBOX_UTF8_TO_UTF16("sdl", &sessionType); - } + displayutf8 = g_strdup_printf("DISPLAY=%s", display); + VBOX_UTF8_TO_UTF16(displayutf8, &env); + VIR_FREE(displayutf8); + VIR_FREE(guiDisplay);
- if (vrdpPresent) + VBOX_UTF8_TO_UTF16(sessType, &sessionType); + } else if (vrdpPresent) { VBOX_UTF8_TO_UTF16("vrdp", &sessionType); + }
rc = gVBoxAPI.UIMachine.LaunchVMProcess(data, machine, iid, sessionType, env, -- 2.39.1
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Michal Prívozník