[virt-tools-list] [PATCH v2] remote-viewer: fix free on dangling pointer

[Eduardo Lima (Etrunko)]

On 10/1/19 12:51 PM, Victor Toso wrote:
> Hi,
> 
> On Tue, Oct 01, 2019 at 11:17:22AM -0300, Eduardo Lima (Etrunko) wrote:
>> On 10/1/19 7:09 AM, Victor Toso wrote:
>>>> 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.
> 
> Ovirt is not that easy to poke. Couldn't reproduce the warning
> because I'm not able to connect to running ovirt with Fedora 31.
> Tried with master from libgovirt, no luck.

I see, and taking a better look at ovirt in Fedora, it seems very 
outdated with upstream release. I will work on updating the package 
there. In the meantime we can try

> 
> As mentioned in the commit log, passing a g_strdup() + g_free()
> to a signal that can be triggered more than once is problematic
> by itself so, suggestions welcome.
> 

One alternative solution would be usage of g_intern_string() so there is 
no need to free it anymore. Your patch would become something like the 
following:

diff --git a/src/remote-viewer.c b/src/remote-viewer.c
index bd8c2eb..5c7a379 100644
--- a/src/remote-viewer.c
+++ b/src/remote-viewer.c
@@ -657,7 +657,6 @@ remote_viewer_session_connected(VirtViewerSession 
*session,

      remote_viewer_recent_add(uri, mime);
      g_free(uri);
-    g_free(guri);
  }

  static gchar *
@@ -696,7 +695,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), 
(gchar*) g_intern_string(guri));

      virt_viewer_session_set_file(virt_viewer_app_get_session(app), 
vvfile);
  #ifdef HAVE_OVIRT




>>> 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


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - Red Hat
etrunko at redhat.com