Bug 734769 - virt-viewer will reference a NULL pointer if URI parsing fails
Summary: virt-viewer will reference a NULL pointer if URI parsing fails
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: virt-viewer
Version: 6.2
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: ---
Assignee: Daniel Berrangé
QA Contact: Virtualization Bugs
URL:
Whiteboard:
Depends On:
Blocks: 743047
TreeView+ depends on / blocked
 
Reported: 2011-08-31 11:41 UTC by Pavel Raiskup
Modified: 2011-12-06 15:07 UTC (History)
6 users (show)

Fixed In Version: virt-viewer-0.4.1-5.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-06 15:07:29 UTC


Attachments (Terms of Use)
Attach a patch for this issues. (2.09 KB, patch)
2011-09-23 08:37 UTC, Alex Jia
kdudka: review-
Details | Diff
updating patch. (2.08 KB, patch)
2011-09-23 11:14 UTC, Alex Jia
no flags Details | Diff


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHEA-2011:1614 normal SHIPPED_LIVE virt-viewer bug fix and enhancement update 2011-12-06 00:51:05 UTC

Description Pavel Raiskup 2011-08-31 11:41:13 UTC
Null dereference in virt_viewer_util_extract_host() 

=> src/virt-viewer-util.c:81/88/90

assigning uri = xmlParseURI(uristr); may cause uri to be NULL
when xmlParseURI fails and there are some cases (mentioned above) which are not
checking this.

Comment 2 Daniel Berrangé 2011-09-19 10:56:21 UTC
Fix available upstream

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

    Return if xmlParseURI() failed, instead of crashing

Comment 4 Daniel Berrangé 2011-09-19 11:02:19 UTC
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.

Comment 11 Alex Jia 2011-09-23 08:37:12 UTC
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 12 Kamil Dudka 2011-09-23 09:28:06 UTC
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

Comment 14 Kamil Dudka 2011-09-23 10:00:10 UTC
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.

Comment 15 Kamil Dudka 2011-09-23 10:08:40 UTC
> '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);
}

Comment 16 Alex Jia 2011-09-23 10:46:45 UTC
(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

Comment 17 Alex Jia 2011-09-23 11:14:21 UTC
Created attachment 524591 [details]
updating patch.

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

Comment 18 Daniel Berrangé 2011-09-28 16:29:18 UTC
@ajia this additional coverity problem has been addressed as part of the fix for bug 740724

Comment 19 Pavel Raiskup 2011-10-07 09:18:37 UTC
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.

Comment 20 Alex Jia 2011-10-08 05:54:46 UTC
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

Comment 21 errata-xmlrpc 2011-12-06 15:07:29 UTC
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


Note You need to log in before you can comment on or make changes to this bug.