On 04/13/2017 10:43 AM, Max Reitz wrote: > The block layer takes care of removing the bs->file child if the block > driver's bdrv_open()/bdrv_file_open() implementation fails. The block > driver therefore does not need to do so, and indeed should not unless it > sets bs->file to NULL afterwards -- because if this is not done, the > bdrv_unref_child() in bdrv_open_inherit() will dereference the freed > memory block at bs->file afterwards, which is not good. > > We can now decide whether to add a "bs->file = NULL;" after each of the > offending bdrv_unref_child() invocations, or just drop them altogether. > The latter is simpler, so let's do that. > > Cc: qemu-stable <qemu-sta...@nongnu.org> > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > It's an issue only in blkdebug, blkreplace and blkverify, and only when > an error occurs in their open functions; therefore I think this is fine > to delay until 2.10. > > However, it *is* a use-after-free newly introduced in 2.9, so that's > where the question mark comes from... > --- > block/blkdebug.c | 4 +--- > block/blkreplay.c | 3 --- > block/blkverify.c | 3 --- > 3 files changed, 1 insertion(+), 9 deletions(-)
On its own, this patch is not worthy of an -rc5 (it only affects failure
paths, and 2 of the 3 affected drivers are not for production use). But
use-after-free is annoying, and the fact that blkreplay is affected
could trip up a user that mistypes arguments causing a BD open to fail.
I'd argue that since we're looking at -rc5 anyways, this is fair game
for inclusion in that build, since it is a small enough patch and easy
to review for correctness. Hence I changed the subject in my reply to
'2.9?'
But I won't lose any sleep if we just wait for 2.9.1 and 2.10.
Reviewed-by: Eric Blake <ebl...@redhat.com>
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 67e8024e36..cc4a146e84 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -389,14 +389,12 @@ static int blkdebug_open(BlockDriverState *bs, QDict
> *options, int flags,
> } else if (align) {
> error_setg(errp, "Invalid alignment");
> ret = -EINVAL;
> - goto fail_unref;
> + goto out;
> }
>
And this means I get to rebase my blkdebug patches on top of yours, if
yours goes in first.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
