| Message ID | 20220427100116.30453-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-user-scsi: avoid unlink(NULL) with fd passing | expand |
On Wed, Apr 27, 2022 at 11:01:16AM +0100, Stefan Hajnoczi wrote: > Commit 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend > Program conventions for vhost-user-scsi") introduced fd-passing support > as part of implementing the vhost-user backend program conventions. > > When fd passing is used the UNIX domain socket path is NULL and we must > not call unlink(2). > > Fixes: Coverity CID 1488353 > Fixes: 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend Program conventions for vhost-user-scsi") > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > --- > contrib/vhost-user-scsi/vhost-user-scsi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c > index b2c0f98253..08335d4b2b 100644 > --- a/contrib/vhost-user-scsi/vhost-user-scsi.c > +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c > @@ -433,7 +433,9 @@ out: > if (vdev_scsi) { > g_main_loop_unref(vdev_scsi->loop); > g_free(vdev_scsi); > - unlink(opt_socket_path); > + if (opt_socket_path) { > + unlink(opt_socket_path); > + } > } > if (csock >= 0) { > close(csock); > -- > 2.35.1 >
On 27/4/22 12:01, Stefan Hajnoczi wrote: > Commit 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend > Program conventions for vhost-user-scsi") introduced fd-passing support > as part of implementing the vhost-user backend program conventions. > > When fd passing is used the UNIX domain socket path is NULL and we must > not call unlink(2). > > Fixes: Coverity CID 1488353 > Fixes: 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend Program conventions for vhost-user-scsi") > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > contrib/vhost-user-scsi/vhost-user-scsi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Wed, 27 Apr 2022 at 11:04, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > Commit 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend > Program conventions for vhost-user-scsi") introduced fd-passing support > as part of implementing the vhost-user backend program conventions. > > When fd passing is used the UNIX domain socket path is NULL and we must > not call unlink(2). > > Fixes: Coverity CID 1488353 > Fixes: 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend Program conventions for vhost-user-scsi") > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > contrib/vhost-user-scsi/vhost-user-scsi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c > index b2c0f98253..08335d4b2b 100644 > --- a/contrib/vhost-user-scsi/vhost-user-scsi.c > +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c > @@ -433,7 +433,9 @@ out: > if (vdev_scsi) { > g_main_loop_unref(vdev_scsi->loop); > g_free(vdev_scsi); > - unlink(opt_socket_path); > + if (opt_socket_path) { > + unlink(opt_socket_path); > + } > } Shouldn't this check-and-unlink be one level up, outside the "if (vdev_scsi)" ? There are error exit paths which get us to the 'out:' label where we have called unix_sock_new() but not yet done the g_new0() of vdev_scsi(). The only thing that needs to be guarded by "if (vdev_scsi)" is the g_main_loop_unref() (the g_free of vdev_scsi itself could be inside or outside, since g_free(NULL) is a nop). thanks -- PMM
On Thu, May 12, 2022 at 04:57:13PM +0100, Peter Maydell wrote: > On Wed, 27 Apr 2022 at 11:04, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > Commit 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend > > Program conventions for vhost-user-scsi") introduced fd-passing support > > as part of implementing the vhost-user backend program conventions. > > > > When fd passing is used the UNIX domain socket path is NULL and we must > > not call unlink(2). > > > > Fixes: Coverity CID 1488353 > > Fixes: 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend Program conventions for vhost-user-scsi") > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > contrib/vhost-user-scsi/vhost-user-scsi.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c > > index b2c0f98253..08335d4b2b 100644 > > --- a/contrib/vhost-user-scsi/vhost-user-scsi.c > > +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c > > @@ -433,7 +433,9 @@ out: > > if (vdev_scsi) { > > g_main_loop_unref(vdev_scsi->loop); > > g_free(vdev_scsi); > > - unlink(opt_socket_path); > > + if (opt_socket_path) { > > + unlink(opt_socket_path); > > + } > > } > > Shouldn't this check-and-unlink be one level up, outside the > "if (vdev_scsi)" ? There are error exit paths which get us to > the 'out:' label where we have called unix_sock_new() but > not yet done the g_new0() of vdev_scsi(). The only thing > that needs to be guarded by "if (vdev_scsi)" is the > g_main_loop_unref() (the g_free of vdev_scsi itself could > be inside or outside, since g_free(NULL) is a nop). > > thanks > -- PMM Stefan, want to respond?
On Thu, May 12, 2022 at 04:57:13PM +0100, Peter Maydell wrote: > On Wed, 27 Apr 2022 at 11:04, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > Commit 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend > > Program conventions for vhost-user-scsi") introduced fd-passing support > > as part of implementing the vhost-user backend program conventions. > > > > When fd passing is used the UNIX domain socket path is NULL and we must > > not call unlink(2). > > > > Fixes: Coverity CID 1488353 > > Fixes: 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend Program conventions for vhost-user-scsi") > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > contrib/vhost-user-scsi/vhost-user-scsi.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c > > index b2c0f98253..08335d4b2b 100644 > > --- a/contrib/vhost-user-scsi/vhost-user-scsi.c > > +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c > > @@ -433,7 +433,9 @@ out: > > if (vdev_scsi) { > > g_main_loop_unref(vdev_scsi->loop); > > g_free(vdev_scsi); > > - unlink(opt_socket_path); > > + if (opt_socket_path) { > > + unlink(opt_socket_path); > > + } > > } > > Shouldn't this check-and-unlink be one level up, outside the > "if (vdev_scsi)" ? There are error exit paths which get us to > the 'out:' label where we have called unix_sock_new() but > not yet done the g_new0() of vdev_scsi(). The only thing > that needs to be guarded by "if (vdev_scsi)" is the > g_main_loop_unref() (the g_free of vdev_scsi itself could > be inside or outside, since g_free(NULL) is a nop). Sorry, I was offline last week due to illness. Now I'm back and agree with what you found. I have sent a v2. Stefan
diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c index b2c0f98253..08335d4b2b 100644 --- a/contrib/vhost-user-scsi/vhost-user-scsi.c +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c @@ -433,7 +433,9 @@ out: if (vdev_scsi) { g_main_loop_unref(vdev_scsi->loop); g_free(vdev_scsi); - unlink(opt_socket_path); + if (opt_socket_path) { + unlink(opt_socket_path); + } } if (csock >= 0) { close(csock);
Commit 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend Program conventions for vhost-user-scsi") introduced fd-passing support as part of implementing the vhost-user backend program conventions. When fd passing is used the UNIX domain socket path is NULL and we must not call unlink(2). Fixes: Coverity CID 1488353 Fixes: 747421e949fc1eb3ba66b5fcccdb7ba051918241 ("Implements Backend Program conventions for vhost-user-scsi") Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/vhost-user-scsi/vhost-user-scsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)