Bug 734769 - virt-viewer will reference a NULL pointer if URI parsing fails
virt-viewer will reference a NULL pointer if URI parsing fails
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: virt-viewer (Show other bugs)
6.2
Unspecified Unspecified
unspecified Severity unspecified
: rc
: ---
Assigned To: Daniel Berrange
Virtualization Bugs
: Regression
Depends On:
Blocks: 743047
  Show dependency treegraph
 
Reported: 2011-08-31 07:41 EDT by Pavel Raiskup
Modified: 2011-12-06 10:07 EST (History)
6 users (show)

See Also:
Fixed In Version: virt-viewer-0.4.1-5.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-12-06 10:07:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


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

  None (edit)
Description Pavel Raiskup 2011-08-31 07:41:13 EDT
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 Berrange 2011-09-19 06:56:21 EDT
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 Berrange 2011-09-19 07:02:19 EDT
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 04:37:12 EDT
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 05:28:06 EDT
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 06:00:10 EDT
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 06:08:40 EDT
> '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 06:46:45 EDT
(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 07:14:21 EDT
Created attachment 524591 [details]
updating patch.

Hope this patch is right, and can resolve all of issues.
Comment 18 Daniel Berrange 2011-09-28 12:29:18 EDT
@ajia this additional coverity problem has been addressed as part of the fix for bug 740724
Comment 19 Pavel Raiskup 2011-10-07 05:18:37 EDT
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 01:54:46 EDT
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 10:07:29 EST
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.