| Message ID | alpine.DEB.2.10.1705081334150.24729@sstabellini-ThinkPad-X260 |
|---|---|
| State | New |
| Headers | show |
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.
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 --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,
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>