| Message ID | ebdf772d1a48ccec999866f9c5cb209b2fcedf48.1502144886.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/07/2017 06:29 PM, Jeff Cody wrote: > Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion > checks for when qemu_mutex() functions are called without the > corresponding qemu_mutex_init() having initialized the mutex. > > This uncovered a latent bug in qemu's nfs driver - in > nfs_client_close(), the NFSClient structure is overwritten with zeros, > prior to the mutex being destroyed. > > Go ahead and destroy the mutex in nfs_client_close(), and change where > we call qemu_mutex_init() so that it is correctly balanced. > > There are also a couple of memory leaks obscured by the memset, so this > fixes those as well. > > Finally, we should be able to get rid of the memset(), as it isn't > necessary. > > Signed-off-by: Jeff Cody <jcody@redhat.com> Looks sane to me. Reviewed-by: John Snow <jsnow@redhat.com>
On Mon, Aug 07, 2017 at 06:29:09PM -0400, Jeff Cody wrote: > Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion > checks for when qemu_mutex() functions are called without the > corresponding qemu_mutex_init() having initialized the mutex. > > This uncovered a latent bug in qemu's nfs driver - in > nfs_client_close(), the NFSClient structure is overwritten with zeros, > prior to the mutex being destroyed. > > Go ahead and destroy the mutex in nfs_client_close(), and change where > we call qemu_mutex_init() so that it is correctly balanced. > > There are also a couple of memory leaks obscured by the memset, so this > fixes those as well. > > Finally, we should be able to get rid of the memset(), as it isn't > necessary. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/nfs.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Am 08.08.2017 um 00:29 schrieb Jeff Cody: > Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion > checks for when qemu_mutex() functions are called without the > corresponding qemu_mutex_init() having initialized the mutex. > > This uncovered a latent bug in qemu's nfs driver - in > nfs_client_close(), the NFSClient structure is overwritten with zeros, > prior to the mutex being destroyed. > > Go ahead and destroy the mutex in nfs_client_close(), and change where > we call qemu_mutex_init() so that it is correctly balanced. > > There are also a couple of memory leaks obscured by the memset, so this > fixes those as well. > > Finally, we should be able to get rid of the memset(), as it isn't > necessary. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/nfs.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/block/nfs.c b/block/nfs.c > index d8db419..bec16b7 100644 > --- a/block/nfs.c > +++ b/block/nfs.c > @@ -433,19 +433,23 @@ static void nfs_client_close(NFSClient *client) > if (client->context) { > if (client->fh) { > nfs_close(client->context, client->fh); > + client->fh = NULL; > } > aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context), > false, NULL, NULL, NULL, NULL); > nfs_destroy_context(client->context); > + client->context = NULL; > } > - memset(client, 0, sizeof(NFSClient)); > -} > - > -static void nfs_file_close(BlockDriverState *bs) > -{ > - NFSClient *client = bs->opaque; > - nfs_client_close(client); > + g_free(client->path); > qemu_mutex_destroy(&client->mutex); > + qapi_free_NFSServer(client->server); > + client->server = NULL; > +} > + > +static void nfs_file_close(BlockDriverState *bs) > +{ > + NFSClient *client = bs->opaque; > + nfs_client_close(client); > } > > static NFSServer *nfs_config(QDict *options, Error **errp) > @@ -498,6 +502,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options, > struct stat st; > char *file = NULL, *strp = NULL; > > + qemu_mutex_init(&client->mutex); > opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > qemu_opts_absorb_qdict(opts, options, &local_err); > if (local_err) { > @@ -660,7 +665,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags, > if (ret < 0) { > return ret; > } > - qemu_mutex_init(&client->mutex); > + > bs->total_sectors = ret; > ret = 0; > return ret; Reviewed-by: Peter Lieven <pl@kamp.de> Also CC'ing qemu-stable as this affects 2.9.0 as well. Peter
Am 08.08.2017 um 00:29 hat Jeff Cody geschrieben: > Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion > checks for when qemu_mutex() functions are called without the > corresponding qemu_mutex_init() having initialized the mutex. > > This uncovered a latent bug in qemu's nfs driver - in > nfs_client_close(), the NFSClient structure is overwritten with zeros, > prior to the mutex being destroyed. > > Go ahead and destroy the mutex in nfs_client_close(), and change where > we call qemu_mutex_init() so that it is correctly balanced. > > There are also a couple of memory leaks obscured by the memset, so this > fixes those as well. > > Finally, we should be able to get rid of the memset(), as it isn't > necessary. > > Signed-off-by: Jeff Cody <jcody@redhat.com> Thanks, applied to the block branch. Kevin
diff --git a/block/nfs.c b/block/nfs.c index d8db419..bec16b7 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -433,19 +433,23 @@ static void nfs_client_close(NFSClient *client) if (client->context) { if (client->fh) { nfs_close(client->context, client->fh); + client->fh = NULL; } aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context), false, NULL, NULL, NULL, NULL); nfs_destroy_context(client->context); + client->context = NULL; } - memset(client, 0, sizeof(NFSClient)); -} - -static void nfs_file_close(BlockDriverState *bs) -{ - NFSClient *client = bs->opaque; - nfs_client_close(client); + g_free(client->path); qemu_mutex_destroy(&client->mutex); + qapi_free_NFSServer(client->server); + client->server = NULL; +} + +static void nfs_file_close(BlockDriverState *bs) +{ + NFSClient *client = bs->opaque; + nfs_client_close(client); } static NFSServer *nfs_config(QDict *options, Error **errp) @@ -498,6 +502,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options, struct stat st; char *file = NULL, *strp = NULL; + qemu_mutex_init(&client->mutex); opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); qemu_opts_absorb_qdict(opts, options, &local_err); if (local_err) { @@ -660,7 +665,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags, if (ret < 0) { return ret; } - qemu_mutex_init(&client->mutex); + bs->total_sectors = ret; ret = 0; return ret;
Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion checks for when qemu_mutex() functions are called without the corresponding qemu_mutex_init() having initialized the mutex. This uncovered a latent bug in qemu's nfs driver - in nfs_client_close(), the NFSClient structure is overwritten with zeros, prior to the mutex being destroyed. Go ahead and destroy the mutex in nfs_client_close(), and change where we call qemu_mutex_init() so that it is correctly balanced. There are also a couple of memory leaks obscured by the memset, so this fixes those as well. Finally, we should be able to get rid of the memset(), as it isn't necessary. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/nfs.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)