diff mbox

Fix issues affecting Xen 9pfs discovered by Coverity

Message ID alpine.DEB.2.10.1705081334150.24729@sstabellini-ThinkPad-X260 (mailing list archive)
State New, archived
Headers show

Commit Message

Stefano Stabellini May 8, 2017, 8:45 p.m. UTC 
Fix two resource leaks on error paths, discovered by Coverity.
Check for errors returned by fcntl, also found by Coverity.

CID:1374836
CID:1374831

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

Comments

Eric Blake May 8, 2017, 9:11 p.m. UTC | #1 
On 05/08/2017 03:45 PM, Stefano Stabellini wrote:
> Fix two resource leaks on error paths, discovered by Coverity.
> Check for errors returned by fcntl, also found by Coverity.
> 
> CID:1374836
> CID:1374831
> 

> @@ -378,7 +380,10 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
>          if (xen_9pdev->rings[i].evtchndev == NULL) {
>              goto out;
>          }
> -        fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev), F_SETFD, FD_CLOEXEC);
> +        if (fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
> +                  F_SETFD, FD_CLOEXEC) == -1) {
> +            goto out;

Directly calling fcntl(F_SETFD) without first reading fcntl(F_GETFD) is
(theoretically) incorrect.  Better might be using qemu_set_cloexec()
instead of open-coding something.
Stefano Stabellini May 8, 2017, 9:57 p.m. UTC | #2 
On Mon, 8 May 2017, Eric Blake wrote:
> On 05/08/2017 03:45 PM, Stefano Stabellini wrote:
> > Fix two resource leaks on error paths, discovered by Coverity.
> > Check for errors returned by fcntl, also found by Coverity.
> > 
> > CID:1374836
> > CID:1374831
> > 
> 
> > @@ -378,7 +380,10 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
> >          if (xen_9pdev->rings[i].evtchndev == NULL) {
> >              goto out;
> >          }
> > -        fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev), F_SETFD, FD_CLOEXEC);
> > +        if (fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
> > +                  F_SETFD, FD_CLOEXEC) == -1) {
> > +            goto out;
> 
> Directly calling fcntl(F_SETFD) without first reading fcntl(F_GETFD) is
> (theoretically) incorrect.  Better might be using qemu_set_cloexec()
> instead of open-coding something.

Makes sense but the unchecked return of fcntl, discovered by Coverity,
would remain unfixed by calling qemu_set_cloexec here. I don't think I
am up for fixing all the call sites of qemu_set_cloexec.

I am going to drop this change, and resend this patch was only the other
two fixes, fixing 1374836 only.
diff mbox

Patch

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 9c7f41a..f75e728 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -332,12 +332,14 @@  static int xen_9pfs_connect(struct XenDevice *xendev)
         str = g_strdup_printf("ring-ref%u", i);
         if (xenstore_read_fe_int(&xen_9pdev->xendev, str,
                                  &xen_9pdev->rings[i].ref) == -1) {
+            g_free(str);
             goto out;
         }
         g_free(str);
         str = g_strdup_printf("event-channel-%u", i);
         if (xenstore_read_fe_int(&xen_9pdev->xendev, str,
                                  &xen_9pdev->rings[i].evtchn) == -1) {
+            g_free(str);
             goto out;
         }
         g_free(str);
@@ -378,7 +380,10 @@  static int xen_9pfs_connect(struct XenDevice *xendev)
         if (xen_9pdev->rings[i].evtchndev == NULL) {
             goto out;
         }
-        fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev), F_SETFD, FD_CLOEXEC);
+        if (fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
+                  F_SETFD, FD_CLOEXEC) == -1) {
+            goto out;
+        }
         xen_9pdev->rings[i].local_port = xenevtchn_bind_interdomain
                                             (xen_9pdev->rings[i].evtchndev,
                                              xendev->dom,