| Message ID | 20170701153906.16588-1-el13635@mail.ntua.gr |
|---|---|
| State | New |
| Headers | show |
Am 01.07.2017 um 17:39 hat Manos Pitsidianakis geschrieben: > bdrv_open_driver() is called in two places, bdrv_new_open_driver() and > bdrv_open_common(). In the latter, failure cleanup in is in its caller, > bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it > exists. > > Let's move the bs->file cleanup to bdrv_open_driver() to take care of all > callers and do not set bs->drv to NULL unless the driver's open function > failed. When bs is destroyed by removing its last reference, bdrv_close() > checks bs->drv to perform the needed cleanups and also call the driver's close > function. > > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > --- > > v2: > move bdrv_unref_child(bs, bs->file) to bdrv_open_driver > do not set bs->drv to NULL if open succeeds > > block.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/block.c b/block.c > index 694396281b..df2a46990c 100644 > --- a/block.c > +++ b/block.c > @@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > { > Error *local_err = NULL; > int ret; > + bool open_failed; > > bdrv_assign_node_name(bs, node_name, &local_err); > if (local_err) { > @@ -1111,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > ret = 0; > } > > - if (ret < 0) { > + open_failed = ret < 0; > + > + if (open_failed) { > if (local_err) { > error_propagate(errp, local_err); > } else if (bs->filename[0]) { > @@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > return 0; > > free_and_fail: > - /* FIXME Close bs first if already opened*/ > - g_free(bs->opaque); > - bs->opaque = NULL; > - bs->drv = NULL; > + if (open_failed) { > + g_free(bs->opaque); > + bs->opaque = NULL; > + bs->drv = NULL; > + } > + if (bs->file != NULL) { > + bdrv_unref_child(bs, bs->file); > + bs->file = NULL; > + } Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers expect that if an image is opened, it also has a valid bs->file. For example, if I add ret = -1 after refresh_total_sectors() (because I couldn't find an easier way to make it fail intentionally), I get an ugly heap corruption crash instead of a nice error message with this patch. Kevin
On Tue, Jul 11, 2017 at 05:16:17PM +0200, Kevin Wolf wrote: >Am 01.07.2017 um 17:39 hat Manos Pitsidianakis geschrieben: >> bdrv_open_driver() is called in two places, bdrv_new_open_driver() and >> bdrv_open_common(). In the latter, failure cleanup in is in its caller, >> bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it >> exists. >> >> Let's move the bs->file cleanup to bdrv_open_driver() to take care of all >> callers and do not set bs->drv to NULL unless the driver's open function >> failed. When bs is destroyed by removing its last reference, bdrv_close() >> checks bs->drv to perform the needed cleanups and also call the driver's close >> function. >> >> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> >> --- >> >> v2: >> move bdrv_unref_child(bs, bs->file) to bdrv_open_driver >> do not set bs->drv to NULL if open succeeds >> >> block.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/block.c b/block.c >> index 694396281b..df2a46990c 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >> { >> Error *local_err = NULL; >> int ret; >> + bool open_failed; >> >> bdrv_assign_node_name(bs, node_name, &local_err); >> if (local_err) { >> @@ -1111,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >> ret = 0; >> } >> >> - if (ret < 0) { >> + open_failed = ret < 0; >> + >> + if (open_failed) { >> if (local_err) { >> error_propagate(errp, local_err); >> } else if (bs->filename[0]) { >> @@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >> return 0; >> >> free_and_fail: >> - /* FIXME Close bs first if already opened*/ >> - g_free(bs->opaque); >> - bs->opaque = NULL; >> - bs->drv = NULL; >> + if (open_failed) { >> + g_free(bs->opaque); >> + bs->opaque = NULL; >> + bs->drv = NULL; >> + } >> + if (bs->file != NULL) { >> + bdrv_unref_child(bs, bs->file); >> + bs->file = NULL; >> + } > >Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers >expect that if an image is opened, it also has a valid bs->file. > >For example, if I add ret = -1 after refresh_total_sectors() (because I >couldn't find an easier way to make it fail intentionally), I get an >ugly heap corruption crash instead of a nice error message with this >patch. > This is triggered by bdrv_open_inherit doing QDECREF(bs->explicit_options) and leaving the dangling pointer. Not setting bs->drv means bdrv_close was called and tried to decref it again, causing the heap error. Setting bs->explicit_options = NULL; right below that fixes the heap corruption for me. I can send a seperate fix for this. I also saw that there's no reason to use a boolean, a label would do just fine so I can change that and finalize the patch in the next version if everything is okay with it.
Am 11.07.2017 um 20:50 hat Manos Pitsidianakis geschrieben: > On Tue, Jul 11, 2017 at 05:16:17PM +0200, Kevin Wolf wrote: > >Am 01.07.2017 um 17:39 hat Manos Pitsidianakis geschrieben: > >>bdrv_open_driver() is called in two places, bdrv_new_open_driver() and > >>bdrv_open_common(). In the latter, failure cleanup in is in its caller, > >>bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it > >>exists. > >> > >>Let's move the bs->file cleanup to bdrv_open_driver() to take care of all > >>callers and do not set bs->drv to NULL unless the driver's open function > >>failed. When bs is destroyed by removing its last reference, bdrv_close() > >>checks bs->drv to perform the needed cleanups and also call the driver's close > >>function. > >> > >>Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > >>--- > >> > >>v2: > >> move bdrv_unref_child(bs, bs->file) to bdrv_open_driver > >> do not set bs->drv to NULL if open succeeds > >> > >> block.c | 21 +++++++++++++-------- > >> 1 file changed, 13 insertions(+), 8 deletions(-) > >> > >>diff --git a/block.c b/block.c > >>index 694396281b..df2a46990c 100644 > >>--- a/block.c > >>+++ b/block.c > >>@@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > >> { > >> Error *local_err = NULL; > >> int ret; > >>+ bool open_failed; > >> > >> bdrv_assign_node_name(bs, node_name, &local_err); > >> if (local_err) { > >>@@ -1111,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > >> ret = 0; > >> } > >> > >>- if (ret < 0) { > >>+ open_failed = ret < 0; > >>+ > >>+ if (open_failed) { > >> if (local_err) { > >> error_propagate(errp, local_err); > >> } else if (bs->filename[0]) { > >>@@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, > >> return 0; > >> > >> free_and_fail: > >>- /* FIXME Close bs first if already opened*/ > >>- g_free(bs->opaque); > >>- bs->opaque = NULL; > >>- bs->drv = NULL; > >>+ if (open_failed) { > >>+ g_free(bs->opaque); > >>+ bs->opaque = NULL; > >>+ bs->drv = NULL; > >>+ } > >>+ if (bs->file != NULL) { > >>+ bdrv_unref_child(bs, bs->file); > >>+ bs->file = NULL; > >>+ } > > > >Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers > >expect that if an image is opened, it also has a valid bs->file. > > > >For example, if I add ret = -1 after refresh_total_sectors() (because I > >couldn't find an easier way to make it fail intentionally), I get an > >ugly heap corruption crash instead of a nice error message with this > >patch. > > > This is triggered by bdrv_open_inherit doing > QDECREF(bs->explicit_options) and leaving the dangling pointer. Not > setting bs->drv means bdrv_close was called and tried to decref it > again, causing the heap error. Setting bs->explicit_options = NULL; > right below that fixes the heap corruption for me. Wouldn't it be better to call drv->bdrv_close() instead and then set bs->drv/opaque = NULL like for the other error path? > I can send a seperate fix for this. No, this doesn't fail before this patch, so it's a regression and we can't merge the patch without a fix. You need to respin this one. > I also saw that there's no reason to use a boolean, a label would do > just fine so I can change that and finalize the patch in the next > version if everything is okay with it. Yes, that sounds better. Kevin
On Wed, Jul 12, 2017 at 10:33:37AM +0200, Kevin Wolf wrote: >Am 11.07.2017 um 20:50 hat Manos Pitsidianakis geschrieben: >> On Tue, Jul 11, 2017 at 05:16:17PM +0200, Kevin Wolf wrote: >> >Am 01.07.2017 um 17:39 hat Manos Pitsidianakis geschrieben: >> >>bdrv_open_driver() is called in two places, bdrv_new_open_driver() and >> >>bdrv_open_common(). In the latter, failure cleanup in is in its caller, >> >>bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it >> >>exists. >> >> >> >>Let's move the bs->file cleanup to bdrv_open_driver() to take care of all >> >>callers and do not set bs->drv to NULL unless the driver's open function >> >>failed. When bs is destroyed by removing its last reference, bdrv_close() >> >>checks bs->drv to perform the needed cleanups and also call the driver's close >> >>function. >> >> >> >>Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> >> >>--- >> >> >> >>v2: >> >> move bdrv_unref_child(bs, bs->file) to bdrv_open_driver >> >> do not set bs->drv to NULL if open succeeds >> >> >> >> block.c | 21 +++++++++++++-------- >> >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> >> >>diff --git a/block.c b/block.c >> >>index 694396281b..df2a46990c 100644 >> >>--- a/block.c >> >>+++ b/block.c >> >>@@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >> >> { >> >> Error *local_err = NULL; >> >> int ret; >> >>+ bool open_failed; >> >> >> >> bdrv_assign_node_name(bs, node_name, &local_err); >> >> if (local_err) { >> >>@@ -1111,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >> >> ret = 0; >> >> } >> >> >> >>- if (ret < 0) { >> >>+ open_failed = ret < 0; >> >>+ >> >>+ if (open_failed) { >> >> if (local_err) { >> >> error_propagate(errp, local_err); >> >> } else if (bs->filename[0]) { >> >>@@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, >> >> return 0; >> >> >> >> free_and_fail: >> >>- /* FIXME Close bs first if already opened*/ >> >>- g_free(bs->opaque); >> >>- bs->opaque = NULL; >> >>- bs->drv = NULL; >> >>+ if (open_failed) { >> >>+ g_free(bs->opaque); >> >>+ bs->opaque = NULL; >> >>+ bs->drv = NULL; >> >>+ } >> >>+ if (bs->file != NULL) { >> >>+ bdrv_unref_child(bs, bs->file); >> >>+ bs->file = NULL; >> >>+ } >> > >> >Is this bdrv_unref_child() safe if we leave bs->drv set? Format drivers >> >expect that if an image is opened, it also has a valid bs->file. >> > >> >For example, if I add ret = -1 after refresh_total_sectors() (because I >> >couldn't find an easier way to make it fail intentionally), I get an >> >ugly heap corruption crash instead of a nice error message with this >> >patch. >> > >> This is triggered by bdrv_open_inherit doing >> QDECREF(bs->explicit_options) and leaving the dangling pointer. Not >> setting bs->drv means bdrv_close was called and tried to decref it >> again, causing the heap error. Setting bs->explicit_options = NULL; >> right below that fixes the heap corruption for me. > >Wouldn't it be better to call drv->bdrv_close() instead and then set >bs->drv/opaque = NULL like for the other error path? That was my first approach but I thought it wouldn't look nice since bdrv_close is called anyway on delete. I will do it in the next version. > >> I can send a seperate fix for this. > >No, this doesn't fail before this patch, so it's a regression and we >can't merge the patch without a fix. You need to respin this one. Yes, I meant to first fix this and then apply this patch. It's a dangling pointer anyway. > >> I also saw that there's no reason to use a boolean, a label would do >> just fine so I can change that and finalize the patch in the next >> version if everything is okay with it. > >Yes, that sounds better. > >Kevin
diff --git a/block.c b/block.c index 694396281b..df2a46990c 100644 --- a/block.c +++ b/block.c @@ -1091,6 +1091,7 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, { Error *local_err = NULL; int ret; + bool open_failed; bdrv_assign_node_name(bs, node_name, &local_err); if (local_err) { @@ -1111,7 +1112,9 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, ret = 0; } - if (ret < 0) { + open_failed = ret < 0; + + if (open_failed) { if (local_err) { error_propagate(errp, local_err); } else if (bs->filename[0]) { @@ -1142,10 +1145,15 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, return 0; free_and_fail: - /* FIXME Close bs first if already opened*/ - g_free(bs->opaque); - bs->opaque = NULL; - bs->drv = NULL; + if (open_failed) { + g_free(bs->opaque); + bs->opaque = NULL; + bs->drv = NULL; + } + if (bs->file != NULL) { + bdrv_unref_child(bs, bs->file); + bs->file = NULL; + } return ret; } @@ -2607,9 +2615,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, fail: blk_unref(file); - if (bs->file != NULL) { - bdrv_unref_child(bs, bs->file); - } QDECREF(snapshot_options); QDECREF(bs->explicit_options); QDECREF(bs->options);
bdrv_open_driver() is called in two places, bdrv_new_open_driver() and bdrv_open_common(). In the latter, failure cleanup in is in its caller, bdrv_open_inherit(), which unrefs the bs->file of the failed driver open if it exists. Let's move the bs->file cleanup to bdrv_open_driver() to take care of all callers and do not set bs->drv to NULL unless the driver's open function failed. When bs is destroyed by removing its last reference, bdrv_close() checks bs->drv to perform the needed cleanups and also call the driver's close function. Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> --- v2: move bdrv_unref_child(bs, bs->file) to bdrv_open_driver do not set bs->drv to NULL if open succeeds block.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)