Re: [virt-tools-list] [remote-viewer v1] remote-viewer: fix free on dangling pointer
Hi,
On Fri, Sep 27, 2019 at 04:21:58PM -0300, Eduardo Lima (Etrunko) wrote:
> On 9/27/19 10:35 AM, Victor Toso wrote:
> > static gboolean
> > -create_ovirt_session(VirtViewerApp *app, const char *uri, GError **err)
> > +create_ovirt_session(VirtViewerApp *app, GError **err)
> > {
> > OvirtProxy *proxy = NULL;
> > OvirtApi *api = NULL;
> > @@ -421,9 +421,11 @@ create_ovirt_session(VirtViewerApp *app, const char
> > *uri, GError **err)
> > gchar *ticket = NULL;
> > gchar *host_subject = NULL;
> > gchar *guid = NULL;
> > +gchar *guri = NULL;
> > g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
> > +g_object_get(app, "guri", &guri, NULL);
> > if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username)) {
>
> Patch does not build. Are you sure your configure passes the ovirt checks?
I'm sure that I build without ovirt :)
Fixed, I'll send a v2. Many thanks for checking!
> remote-viewer.c: In function 'create_ovirt_session':
> remote-viewer.c:430:26: error: 'uri' undeclared (first use in this
> function); did you mean 'guri'?
> 430 | if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username)) {
> | ^~~
> | guri
> remote-viewer.c:430:26: note: each undeclared identifier is reported only
> once for each function it appears in
>
>
>
> > g_set_error_literal(&error, VIRT_VIEWER_ERROR,
> > VIRT_VIEWER_ERROR_FAILED,
> > _("failed to parse ovirt uri"));
> > @@ -561,6 +563,7 @@ create_ovirt_session(VirtViewerApp *app, const char
> > *uri, GError **err)
> > success = TRUE;
> > error:
> > +g_free(guri);
> > g_free(username);
> > g_free(rest_uri);
> > g_free(vm_name);
> > @@ -645,17 +648,16 @@ remote_viewer_recent_add(gchar *uri, const gchar
> > *mime_type)
> > static void
> > remote_viewer_session_connected(VirtViewerSession *session,
> > -gchar *guri)
> > +VirtViewerApp *app)
> > {
> > gchar *uri = virt_viewer_session_get_uri(session);
> > const gchar *mime = virt_viewer_session_mime_type(session);
> > if (uri == NULL)
> > -uri = g_strdup(guri);
> > +g_object_get(app, "guri", &uri, NULL);
> > remote_viewer_recent_add(uri, mime);
> > g_free(uri);
> > -g_free(guri);
> > }
> > static gchar *
> > @@ -675,14 +677,14 @@ read_all_stdin(gsize *len, GError **err)
> > }
> > static gboolean
> > -remote_viewer_initial_connect(RemoteViewer *self, const gchar *type, const
> > gchar *guri,
> > +remote_viewer_initial_connect(RemoteViewer *self, const gchar *type,
> > VirtViewerFile *vvfile, GError **error)
> > {
> > VirtViewerApp *app = VIRT_VIEWER_APP(self);
> > #ifdef HAVE_OVIRT
> > if (g_strcmp0(type, "ovirt") == 0) {
> > -if (!create_ovirt_session(app, guri, error)) {
> > +if (!create_ovirt_session(app, error)) {
> > g_prefix_error(error, _("Couldn't open oVirt session: "));
> > return FALSE;
> > }
> > @@ -694,7 +696,7 @@ remote_viewer_initial_connect(RemoteViewer *self, const
> > gchar *type, const gchar
> > }
> > g_signal_connect(virt_viewer_app_get_session(app),
> > "session-connected",
> > - G_CALLBACK(remote_viewer_session_connected),
> > g_strdup(guri));
> > + G_CALLBACK(remote_viewer_session_connected), app);
> > virt_viewer_session_set_file(virt_viewer_app_get_session(app),
> > vvfile);
> > #ifdef HAVE_OVIRT
> > @@ -787,7 +789,7 @@ retry_dialog:
> > _("Cannot determine the connection type
> > from URI"));
> > goto cleanup;
> > }
> > -if (!remote_viewer_initial_connect(self, type, guri, vvfile,
> > &error))
> > +if (!remote_viewer_initial_connect(self, type, vvfile, &error))
> > goto cleanup;
> > }
> >
>
>
> --
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - Red Hat
> etru...@redhat.com
signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [remote-viewer v1] remote-viewer: fix free on dangling pointer
On 9/27/19 10:35 AM, Victor Toso wrote:
From: Victor Toso
On remote_viewer_session_connected() we are passing a dup of URI of
connection and freeing it afterwards. Problem is, we don't disconnect
from listening "session-connected" and on an eventual second emission
of this signal, remote-viewer crashes as seen in the backtrace below.
This can happen over switch-host migration message from
SpiceMainChannel.
To fix the issue, use VirtViewerApp URI information instead of passing
a dup char*. The other changes in this patch were warnings of unused
variables.
Found it while improving migrate.py from spice/tests (server):
| Invalid free() / delete / delete[] / realloc()
|at 0x4839A0C: free (vg_replace_malloc.c:540)
|by 0x56EBD8C: g_free (in /usr/lib64/libglib-2.0.so.0.6000.6)
|by 0x11DED0: remote_viewer_session_connected (remote-viewer.c:658)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x56614F3: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566AF68: g_signal_emit_by_name (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x135E5D: virt_viewer_session_spice_main_channel_event
(virt-viewer-session-spice.c:699)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x56614F3: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x53149E3: emit_main_context (gio-coroutine.c:198)
| Address 0x18f1ecc0 is 0 bytes inside a block of size 23 free'd
|at 0x4839A0C: free (vg_replace_malloc.c:540)
|by 0x56EBD8C: g_free (in /usr/lib64/libglib-2.0.so.0.6000.6)
|by 0x11DED0: remote_viewer_session_connected (remote-viewer.c:658)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x56614F3: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566AF68: g_signal_emit_by_name (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x135E5D: virt_viewer_session_spice_main_channel_event
(virt-viewer-session-spice.c:699)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x56614F3: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x53149E3: emit_main_context (gio-coroutine.c:198)
| Block was alloc'd at
|at 0x483880B: malloc (vg_replace_malloc.c:309)
|by 0x56EBC98: g_malloc (in /usr/lib64/libglib-2.0.so.0.6000.6)
|by 0x5705C43: g_strdup (in /usr/lib64/libglib-2.0.so.0.6000.6)
|by 0x11EB80: remote_viewer_initial_connect (remote-viewer.c:696)
|by 0x11EB80: remote_viewer_start (remote-viewer.c:790)
|by 0x1250D3: virt_viewer_app_start (virt-viewer-app.c:1727)
|by 0x127108: virt_viewer_app_on_application_startup
(virt-viewer-app.c:1870)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x5661638: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A972: g_signal_emit (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x553ECA1: g_application_register (in
/usr/lib64/libgio-2.0.so.0.6000.6)
|by 0x553F41D: g_application_run (in /usr/lib64/libgio-2.0.so.0.6000.6)
Signed-off-by: Victor Toso
---
src/remote-viewer.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/remote-viewer.c b/src/remote-viewer.c
index 8938ef9..2818221 100644
--- a/src/remote-viewer.c
+++ b/src/remote-viewer.c
@@ -396,7 +396,7 @@ virt_viewer_app_set_ovirt_foreign_menu(VirtViewerApp *app,
}
static gboolean
-create_ovirt_session(VirtViewerApp *app, const char *uri, GError **err)
+create_ovirt_session(VirtViewerApp *app, GError **err)
{
OvirtProxy *proxy = NULL;
OvirtApi *api = NULL;
@@ -421,9 +421,11 @@ create_ovirt_session(VirtViewerApp *app, const char *uri,
GError **err)
gchar *ticket = NULL;
gchar *host_subject = NULL;
gchar *guid = NULL;
+gchar *guri = NULL;
g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
+g_object_get(app, "guri", &guri, NULL);
if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username)) {
Patch does not build. Are you sure your configure passes the ovirt checks?
remote-viewer.c: In function 'create_ovirt_session':
remote-viewer.c:430:26: error: 'uri' undeclared (first use in this
function); did you mean 'guri'?
430 | if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username)) {
| ^~~
| guri
remote-viewer.c:430:26: note: each undeclared identifier is reported
only once for each function it appears in
[virt-tools-list] [remote-viewer v1] remote-viewer: fix free on dangling pointer
From: Victor Toso
On remote_viewer_session_connected() we are passing a dup of URI of
connection and freeing it afterwards. Problem is, we don't disconnect
from listening "session-connected" and on an eventual second emission
of this signal, remote-viewer crashes as seen in the backtrace below.
This can happen over switch-host migration message from
SpiceMainChannel.
To fix the issue, use VirtViewerApp URI information instead of passing
a dup char*. The other changes in this patch were warnings of unused
variables.
Found it while improving migrate.py from spice/tests (server):
| Invalid free() / delete / delete[] / realloc()
|at 0x4839A0C: free (vg_replace_malloc.c:540)
|by 0x56EBD8C: g_free (in /usr/lib64/libglib-2.0.so.0.6000.6)
|by 0x11DED0: remote_viewer_session_connected (remote-viewer.c:658)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x56614F3: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566AF68: g_signal_emit_by_name (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x135E5D: virt_viewer_session_spice_main_channel_event
(virt-viewer-session-spice.c:699)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x56614F3: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x53149E3: emit_main_context (gio-coroutine.c:198)
| Address 0x18f1ecc0 is 0 bytes inside a block of size 23 free'd
|at 0x4839A0C: free (vg_replace_malloc.c:540)
|by 0x56EBD8C: g_free (in /usr/lib64/libglib-2.0.so.0.6000.6)
|by 0x11DED0: remote_viewer_session_connected (remote-viewer.c:658)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x56614F3: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566AF68: g_signal_emit_by_name (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x135E5D: virt_viewer_session_spice_main_channel_event
(virt-viewer-session-spice.c:699)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x56614F3: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x53149E3: emit_main_context (gio-coroutine.c:198)
| Block was alloc'd at
|at 0x483880B: malloc (vg_replace_malloc.c:309)
|by 0x56EBC98: g_malloc (in /usr/lib64/libglib-2.0.so.0.6000.6)
|by 0x5705C43: g_strdup (in /usr/lib64/libglib-2.0.so.0.6000.6)
|by 0x11EB80: remote_viewer_initial_connect (remote-viewer.c:696)
|by 0x11EB80: remote_viewer_start (remote-viewer.c:790)
|by 0x1250D3: virt_viewer_app_start (virt-viewer-app.c:1727)
|by 0x127108: virt_viewer_app_on_application_startup
(virt-viewer-app.c:1870)
|by 0x564D741: g_closure_invoke (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x5661638: ??? (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A34D: g_signal_emit_valist (in
/usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x566A972: g_signal_emit (in /usr/lib64/libgobject-2.0.so.0.6000.6)
|by 0x553ECA1: g_application_register (in
/usr/lib64/libgio-2.0.so.0.6000.6)
|by 0x553F41D: g_application_run (in /usr/lib64/libgio-2.0.so.0.6000.6)
Signed-off-by: Victor Toso
---
src/remote-viewer.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/remote-viewer.c b/src/remote-viewer.c
index 8938ef9..2818221 100644
--- a/src/remote-viewer.c
+++ b/src/remote-viewer.c
@@ -396,7 +396,7 @@ virt_viewer_app_set_ovirt_foreign_menu(VirtViewerApp *app,
}
static gboolean
-create_ovirt_session(VirtViewerApp *app, const char *uri, GError **err)
+create_ovirt_session(VirtViewerApp *app, GError **err)
{
OvirtProxy *proxy = NULL;
OvirtApi *api = NULL;
@@ -421,9 +421,11 @@ create_ovirt_session(VirtViewerApp *app, const char *uri,
GError **err)
gchar *ticket = NULL;
gchar *host_subject = NULL;
gchar *guid = NULL;
+gchar *guri = NULL;
g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
+g_object_get(app, "guri", &guri, NULL);
if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username)) {
g_set_error_literal(&error, VIRT_VIEWER_ERROR,
VIRT_VIEWER_ERROR_FAILED,
_("failed to parse ovirt uri"));
@@ -561,6 +563,7 @@ create_ovirt_session(VirtViewerApp *app, const char *uri,
GError **err)
success = TRUE;
error:
+g_free(guri);
g_free(username);
g_free(rest_uri);
g_free(vm_name);
@@ -645,17 +648,16 @@ remote_viewer_recent_add(gchar *uri, const gchar
*mime_type)
static void
remote_viewer_session_connected(VirtViewerSession *session,
-gchar *guri)
+
