Bug 734769

Fix available upstream

commit f08c5308cacda84ab811b85c3cf37ef59383c8c5
Author: Marc-André Lureau <marcandre.lureau>
Date:   Tue Jul 19 19:32:51 2011 +0200

    Return if xmlParseURI() failed, instead of crashing

NB this is a theoretical crash, it is not possible to actually reach this code if the URI is malformed, because libvirt will have already tried xmlParseURI earlier and raised an error message.

Created attachment 524561 [details]
Attach a patch for this issues.

Daniel, I don't know which mail listing accept this patch, so simply attach it to here.

Comment on attachment 524561 [details]
Attach a patch for this issues.

>@@ -330,16 +330,17 @@ virt_viewer_extract_connect_info(VirtViewer *self,
> 	 * from a remote host. Instead we fallback to the hostname used in
> 	 * the libvirt URI. This isn't perfect but it is better than nothing
> 	 */
>-	if (strcmp(ghost, "0.0.0.0") == 0 ||
>-	    strcmp(ghost, "::") == 0) {
>-		DEBUG_LOG("Guest graphics listen '%s' is a wildcard, replacing with '%s'",
>-			  ghost, host);
>-		g_free(ghost);
>-		ghost = g_strdup(host);
>-	}
>-
>-	virt_viewer_app_set_connect_info(app, host, ghost, gport, transport, unixsock, user, port);
>-
>+        if (ghost) {
>+	    if (strcmp(ghost, "0.0.0.0") == 0 ||
>+	        strcmp(ghost, "::") == 0) {
>+		    DEBUG_LOG("Guest graphics listen '%s' is a wildcard, replacing with '%s'",
>+		   	      ghost, host);
>+		    g_free(ghost);
>+		    ghost = g_strdup(host);
>+	    }
>+
>+	    virt_viewer_app_set_connect_info(app, host, ghost, gport, transport, unixsock, user, port);
>+        }

It looks like virt_viewer_app_set_connect_info() should be called out of the if block.  That is, you should either call virt_viewer_app_set_connect_info(), or fail honestly.  You allow to skip that call, yet return TRUE, which feels wrong to me.  Note that a call of g_strdup() on NULL value should be fine according to:

http://developer.gnome.org/glib/2.28/glib-String-Utility-Functions.html#g-strdup

The only access to 'ghost' in virt_viewer_app_set_connect_info() is this:

    priv->ghost = g_strdup(ghost);

So I do not see any problem at this point.  If having ghost == NULL is something invalid (no idea whether it indeed is, I am not a virt-* developer), you need to return failure from virt_viewer_extract_connect_info().  As I said, pretending success without actually updating the connect info could hardly be considered a valid response.

> 'virt_viewer_app_set_connect_info' function will access a NULL 'ghost', is it
> also fine in this step?

Alex, have a look at the virt_viewer_app_free_connect_info() function -- it calls virt_viewer_app_set_connect_info() with a lot of NULLs anyway:

void
virt_viewer_app_free_connect_info(VirtViewerApp *self)
{
    g_return_if_fail(VIRT_VIEWER_IS_APP(self));

    virt_viewer_app_set_connect_info(self, NULL, NULL, NULL, NULL, NULL, NULL, 0);
}

(In reply to comment #15)
Yeah, you're right, in addition, taking 'if (strcmp(ghost, "0.0.0.0") == 0 ...' true branch then 'ghost = g_strdup(host)', so it's also fine.

Thanks,
Alex

Created attachment 524591 [details]
updating patch.

Hope this patch is right, and can resolve all of issues.

@ajia this additional coverity problem has been addressed as part of the fix for bug 740724

I've looked into new Coverity log and both problems (from comment 0 and comment 9) are fixed in patched version virt-viewer-0.4.1-6.

These issues have been resolved on virt-viewer-0.4.1-6, so move this bug to VERIFIED status, the following is latest coverity result:
http://releng-test1.englab.brq.redhat.com/covscan/task/146/

Alex

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHEA-2011-1614.html