[virt-tools-list] [PATCH v2] remote-viewer: fix free on dangling pointer
Eduardo Lima (Etrunko)
etrunko at redhat.com
Tue Oct 1 14:17:22 UTC 2019
On 10/1/19 7:09 AM, Victor Toso wrote:
> Hi,
>
> On Mon, Sep 30, 2019 at 11:01:42AM -0300, Eduardo Lima (Etrunko) wrote:
>> On 9/30/19 5:08 AM, Victor Toso wrote:
>>> From: Victor Toso <me at victortoso.com>
>>>
>>> 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 <victortoso at redhat.com>
>>> ---
>>> 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..7cad231 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 *uri = NULL;
>>> g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
>>> + g_object_get(app, "guri", &uri, 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(uri);
>>> 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);
>>> }
>>
>> Patch builds now, but I started getting some warning when connecting to
>> ovirt:// uri.
>>
>> (remote-viewer:16940): virt-viewer-CRITICAL **: 11:00:14.560:
>> remote_viewer_recent_add: assertion 'uri != NULL' failed
>
> Right, this refactor is not straightforward on ovirt codepath.
> Thanks for testing it properly. I've sent a v3 without touching
> ovirt code now.
>
The warning is still present with your new patch, without it applied, no
warnings are shown.
> Cheers,
>
>>
>>
>>> 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
>> etrunko at redhat.com
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - Red Hat
etrunko at redhat.com
More information about the virt-tools-list
mailing list