diff mbox

[2/2] block: Avoid second open for format probing

Message ID 1352816095-14051-3-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf Nov. 13, 2012, 2:14 p.m. UTC 
This fixes problems that are caused by the additional open/close cycle
of the existing format probing, for example related to qemu-nbd without
-t option or file descriptor passing.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |   58 +++++++++++++++++++++++++++++++---------------------------
 1 files changed, 31 insertions(+), 27 deletions(-)

Comments

Stefan Hajnoczi Nov. 14, 2012, 8:32 a.m. UTC | #1 
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
Paolo Bonzini Nov. 14, 2012, 8:51 a.m. UTC | #2 
Il 14/11/2012 09:32, Stefan Hajnoczi ha scritto:
> 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?

I don't think so.  But do the cases where this happen make sense?  Can
we just fail if drv is not equal to bs->drv if we reach the "if
(drv->bdrv_file_open)" case?  That would be for cases like "-drive
file=test.img,format=host_device" but how does that make sense?...
(Plus, it fails to stack the raw format on top).

So perhaps we could even assert(drv == bs->drv) if protocols had no
.format_name?

> Now if .bdrv_file_open() fails we will bdrv_delete() the already deleted
> file BDS.  This is a double-free.

Right, always better to NULL out whatever you delete (which means
passing a BDS** to bdrv_open_common.

Paolo
Kevin Wolf Nov. 14, 2012, 9:03 a.m. UTC | #3 
Am 14.11.2012 09:32, schrieb Stefan Hajnoczi:
> 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?

No, that looks buggy, even though in practice it doesn't explode at
least with raw-posix. It probably did leak a file descriptor.

> Now if .bdrv_file_open() fails we will bdrv_delete() the already deleted
> file BDS.  This is a double-free.

I'll move the bdrv_delete up into bdrv_open (conditional on bs->file !=
file) and add a file = NULL after it, that should fix it.

Kevin
diff mbox

Patch

diff --git a/block.c b/block.c
index a55b06c..626d6c2 100644
--- a/block.c
+++ b/block.c
@@ -518,22 +518,16 @@  BlockDriver *bdrv_find_protocol(const char *filename)
     return NULL;
 }
 
-static int find_image_format(const char *filename, BlockDriver **pdrv)
+static int find_image_format(BlockDriverState *bs, const char *filename,
+                             BlockDriver **pdrv)
 {
-    int ret, score, score_max;
+    int score, score_max;
     BlockDriver *drv1, *drv;
     uint8_t buf[2048];
-    BlockDriverState *bs;
-
-    ret = bdrv_file_open(&bs, filename, 0);
-    if (ret < 0) {
-        *pdrv = NULL;
-        return ret;
-    }
+    int ret = 0;
 
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
     if (bs->sg || !bdrv_is_inserted(bs)) {
-        bdrv_delete(bs);
         drv = bdrv_find_format("raw");
         if (!drv) {
             ret = -ENOENT;
@@ -543,7 +537,6 @@  static int find_image_format(const char *filename, BlockDriver **pdrv)
     }
 
     ret = bdrv_pread(bs, 0, buf, sizeof(buf));
-    bdrv_delete(bs);
     if (ret < 0) {
         *pdrv = NULL;
         return ret;
@@ -657,7 +650,8 @@  static int bdrv_open_flags(BlockDriverState *bs, int flags)
 /*
  * Common part for opening disk images and files
  */
-static int bdrv_open_common(BlockDriverState *bs, const char *filename,
+static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
+    const char *filename,
     int flags, BlockDriver *drv)
 {
     int ret, open_flags;
@@ -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 {
-        ret = bdrv_file_open(&bs->file, filename, open_flags);
-        if (ret >= 0) {
-            ret = drv->bdrv_open(bs, open_flags);
-        }
+        assert(file != NULL);
+        bs->file = file;
+        ret = drv->bdrv_open(bs, open_flags);
     }
 
     if (ret < 0) {
@@ -716,10 +713,7 @@  static int bdrv_open_common(BlockDriverState *bs, const char *filename,
     return 0;
 
 free_and_fail:
-    if (bs->file) {
-        bdrv_delete(bs->file);
-        bs->file = NULL;
-    }
+    bs->file = NULL;
     g_free(bs->opaque);
     bs->opaque = NULL;
     bs->drv = NULL;
@@ -741,7 +735,7 @@  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
     }
 
     bs = bdrv_new("");
-    ret = bdrv_open_common(bs, filename, flags, drv);
+    ret = bdrv_open_common(bs, NULL, filename, flags, drv);
     if (ret < 0) {
         bdrv_delete(bs);
         return ret;
@@ -795,6 +789,7 @@  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
 {
     int ret;
     char tmp_filename[PATH_MAX];
+    BlockDriverState *file = NULL;
 
     if (flags & BDRV_O_SNAPSHOT) {
         BlockDriverState *bs1;
@@ -854,21 +849,27 @@  int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         bs->is_temporary = 1;
     }
 
+    /* Open image file without format layer */
+    if (flags & BDRV_O_RDWR) {
+        flags |= BDRV_O_ALLOW_RDWR;
+    }
+
+    ret = bdrv_file_open(&file, filename, bdrv_open_flags(bs, flags));
+    if (ret < 0) {
+        return ret;
+    }
+
     /* Find the right image format driver */
     if (!drv) {
-        ret = find_image_format(filename, &drv);
+        ret = find_image_format(file, filename, &drv);
     }
 
     if (!drv) {
         goto unlink_and_fail;
     }
 
-    if (flags & BDRV_O_RDWR) {
-        flags |= BDRV_O_ALLOW_RDWR;
-    }
-
     /* 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);
+    }
     if (bs->is_temporary) {
         unlink(filename);
     }