RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 1655792 - Potential file descriptor leaks from coverity scan
Summary: Potential file descriptor leaks from coverity scan
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 8
Classification: Red Hat
Component: virt-viewer
Version: 8.1
Hardware: Unspecified
OS: Unspecified
low
low
Target Milestone: rc
: 8.0
Assignee: Julien Ropé
QA Contact: zhoujunqin
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-12-03 22:59 UTC by Jonathon Jongsma
Modified: 2020-11-04 03:53 UTC (History)
7 users (show)

Fixed In Version: virt-viewer-9.0-1.el8
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-11-04 03:53:18 UTC
Type: Bug
Target Upstream Version:
Embargoed:
pm-rhel: mirror+


Attachments (Terms of Use)
coverity log (175.79 KB, text/plain)
2018-12-03 23:01 UTC, Jonathon Jongsma
no flags Details
virt-viewer-7.0 (20.23 KB, text/plain)
2020-06-11 13:19 UTC, zhoujunqin
no flags Details
virt-viewer-9.0 (20.79 KB, text/plain)
2020-06-11 13:21 UTC, zhoujunqin
no flags Details
coverity log for virt-viewer-7.0-9.el8 (16.58 KB, text/plain)
2020-06-17 10:39 UTC, zhoujunqin
no flags Details
coverity log for virt-viewer-9.0-2.el8 (1.52 KB, text/plain)
2020-06-17 10:40 UTC, zhoujunqin
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2020:4788 0 None None None 2020-11-04 03:53:42 UTC

Description Jonathon Jongsma 2018-12-03 22:59:56 UTC
The coverity scan of the 7.0-2 build reported potential file descriptor leaks in a couple places. In most cases the session backend will handle closing the file descriptor, but I think there are cases where the spice backend may leak the FD.

Comment 1 Jonathon Jongsma 2018-12-03 23:01:05 UTC
Created attachment 1511139 [details]
coverity log

Comment 2 Julien Ropé 2020-02-20 16:21:12 UTC
 In the Coverity logs, I see 4 occurrences of RESOURCE_LEAK.

 The first and second are in the function "virt_viewer_app_open_tunnel()".
 File descriptors are created by a call to "socketpair()", and after a call to "fork()", the parent keeps one, and the child keeps the other.
 At this point, the child closes its stdin and stdout descriptors, and uses "dup()" to copy its socket and use it as stdin and stdout. Coverity thinks that we're not keeping track of the file descriptor returned by dup(), but since stdin and stdout have just been closed, those duplicated descriptors are replacing them.
 This is a false positive, there is no leak here.


 The third happens in the function "virt_viewer_app_channel_open()".
 At the end of the function, the file descriptor that was previously opened is given as parameter to virt_viewer_session_channel_open_fd(), and Coverity reports that the file descriptor is not used or copied there, and is then lost at the end of the function.
 The call to virt_viewer_session_channel_open_fd() actually ends up calling the function pointer "_VirtViewerSessionClass->channel_open_fd()".
 This pointer is set either to virt_viewer_session_spice_channel_open_fd() or to virt_viewer_session_vnc_channel_open_fd(), depending on the protocol being used.
 - In the case of SPICE, the file descriptor is copied to an internal structure used by the channel later on. If the function succeeds, the file descriptor is not lost, and the caller can ignore this file descriptor. But if the function fails, the file descriptor may stay opened. We are not checking the error code from the call to virt_viewer_session_channel_open_fd(), so we may not be closing the file descriptor in case of an error.
 - In the case of VNC, there is nothing done - a warning actually says "channel_open_fd is not supported by VNC". In this case, the file descriptor is not closed or copied. On return, the file descriptor is then effectively lost.


 The fourth happens in the function virt_viewer_app_default_activate(). It is similar to the third: a socket is opened, and then provided to virt_viewer_session_open_fd(). This is also a call to a pointer function that depends on the protocol:
 - the VNC version is calling a gtk-vnc function with the file descriptor. I didn't verify yet what this function does to the file descriptor in case of failure, but since we are opening it, we're probably supposed to close it.
 - for the SPICE version, the underlying functions being called are the same as in the third item above, with the same result.


 I will add a check for the error code from these functions to make sure we close the file descriptor on errors.

Comment 8 zhoujunqin 2020-06-11 13:19:30 UTC
Created attachment 1696778 [details]
virt-viewer-7.0

Comment 9 zhoujunqin 2020-06-11 13:21:34 UTC
Created attachment 1696780 [details]
virt-viewer-9.0

Comment 14 zhoujunqin 2020-06-17 10:39:35 UTC
Created attachment 1697792 [details]
coverity log for virt-viewer-7.0-9.el8

Comment 15 zhoujunqin 2020-06-17 10:40:29 UTC
Created attachment 1697793 [details]
coverity log for virt-viewer-9.0-2.el8

Comment 17 zhoujunqin 2020-06-18 01:58:46 UTC
Hi @Julien Ropé,
Got it, thanks for your detailed explanation, I move this bug from ON_QA to VERIFIED, thanks.

BR,
juzhou.

Comment 20 errata-xmlrpc 2020-11-04 03:53:18 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 (virt-viewer bug fix and enhancement update), and where to find the updated
files, follow the link below.

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

https://access.redhat.com/errata/RHBA-2020:4788


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