On Tue, Nov 13, 2012 at 03:14:55PM +0100, Kevin Wolf wrote: > @@ -691,12 +685,15 @@ static int bdrv_open_common(BlockDriverState *bs, const > char *filename, > > /* Open the image, either directly or using a protocol */ > if (drv->bdrv_file_open) { > + if (file != NULL) { > + bdrv_swap(file, bs); > + bdrv_delete(file); > + } > ret = drv->bdrv_file_open(bs, filename, open_flags); > } else { [...] > /* Open the image */ > - ret = bdrv_open_common(bs, filename, flags, drv); > + ret = bdrv_open_common(bs, file, filename, flags, drv); > if (ret < 0) { > goto unlink_and_fail; > } > @@ -894,6 +895,9 @@ int bdrv_open(BlockDriverState *bs, const char *filename, > int flags, > return 0; > > unlink_and_fail: > + if (file != NULL) { > + bdrv_delete(file); > + }
Not sure I understand this code path. We have a protocol (the driver implements .bdrv_file_open()) so we swap file and bs, then delete old bs. Then we call .bdrv_file_open() on the already open file BDS. Is it okay to call .bdrv_file_open() on an already open BDS? Now if .bdrv_file_open() fails we will bdrv_delete() the already deleted file BDS. This is a double-free. Stefan