block-commit fixes

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

block.c +17 -6

index e95f613..641b8fa 100644
@@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/* backing_file can either be relative, or absolute.  If it is
+ * relative, it must be relative to the chain.  So, passing in
+ * bs->filename from a BDS as backing_file should not be done,
+ * as that may be relative to the CWD rather than the chain. */
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
         const char *backing_file)
 {
-    if (!bs->drv) {
+    char filename_full[PATH_MAX];
+    char backing_file_full[PATH_MAX];
+    char filename_tmp[PATH_MAX];
+    BlockDriverState *curr_bs = NULL;
+
+    if (!bs || !bs->drv) {
         return NULL;
     }
 
-    if (bs->backing_hd) {
-        if (strcmp(bs->backing_file, backing_file) == 0) {
-            return bs->backing_hd;
-        } else {
-            return bdrv_find_backing_image(bs->backing_hd, backing_file);
+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
+        /* If not an absolute filename path, make it relative to the current
+         * image's filename path */
+        path_combine(filename_tmp, sizeof(filename_tmp),
+                     curr_bs->filename, backing_file);
+
+        /* We are going to compare absolute pathnames */

block.c +5 -0

+            continue;
+        }
+
+        if (strcmp(backing_file_full, filename_full) == 0) {
+            return curr_bs->backing_hd;
         }
     }
 

Il 10/10/2012 07:56, Jeff Cody ha scritto:

Quoted 54 lines

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

block.c +17 -6

index e95f613..641b8fa 100644
@@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/* backing_file can either be relative, or absolute.  If it is
+ * relative, it must be relative to the chain.  So, passing in
+ * bs->filename from a BDS as backing_file should not be done,
+ * as that may be relative to the CWD rather than the chain. */
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
         const char *backing_file)
 {
-    if (!bs->drv) {
+    char filename_full[PATH_MAX];
+    char backing_file_full[PATH_MAX];
+    char filename_tmp[PATH_MAX];
+    BlockDriverState *curr_bs = NULL;
+
+    if (!bs || !bs->drv) {
         return NULL;
     }
 
-    if (bs->backing_hd) {
-        if (strcmp(bs->backing_file, backing_file) == 0) {
-            return bs->backing_hd;
-        } else {
-            return bdrv_find_backing_image(bs->backing_hd, backing_file);
+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
+        /* If not an absolute filename path, make it relative to the current
+         * image's filename path */
+        path_combine(filename_tmp, sizeof(filename_tmp),
+                     curr_bs->filename, backing_file);
+
+        /* We are going to compare absolute pathnames */

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3158

Quoted 10 lines

block.c +10 -0

+        if (!realpath(filename_tmp, filename_full)) {
+            continue;
+        }
+
+        /* We need to make sure the backing filename we are comparing against
+         * is relative to the current image filename (or absolute) */
+        path_combine(filename_tmp, sizeof(filename_tmp),
+                     curr_bs->filename, curr_bs->backing_file);
+
+        if (!realpath(filename_tmp, backing_file_full)) {

Unfortunately realpath does not exist on Win32. But
http://lists.gnu.org/archive/html/grub-devel/2011-09/msg00034.html
suggests that there is a _fullpath and in fact already has all you need to write a qemu_realpath function.

Paolo

Quoted 9 lines

block.c +5 -0

+            continue;
+        }
+
+        if (strcmp(backing_file_full, filename_full) == 0) {
+            return curr_bs->backing_hd;
         }
     }
 

Quoted 4 lines

Unfortunately realpath does not exist on Win32. But
http://lists.gnu.org/archive/html/grub-devel/2011-09/msg00034.html
suggests that there is a _fullpath and in fact already has all you need to write a qemu_realpath function.

Doesn't block.c already rely on realpath(), in bdrv_open()? Why would this be any different using it here?

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

block.c +1 -0

index e95f613..641b8fa 100644
@@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/* backing_file can either be relative, or absolute.  If it is

Quoted 25 lines · Jeff Cody

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

block.c +0 -0

index e95f613..641b8fa 100644
@@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3135

Quoted 11 lines

block.c +7 -1

+/* backing_file can either be relative, or absolute.  If it is
+ * relative, it must be relative to the chain.  So, passing in
+ * bs->filename from a BDS as backing_file should not be done,
+ * as that may be relative to the CWD rather than the chain. */
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
         const char *backing_file)
 {
-    if (!bs->drv) {
+    char filename_full[PATH_MAX];
+    char backing_file_full[PATH_MAX];
+    char filename_tmp[PATH_MAX];

That's a LOT of stack space, which risks stack overflow, will mostly be unused, and still doesn't work if you have super-deep hierarchies larger than PATH_MAX. Would you be better off using realpath(,NULL) for its allocating semantics, and then free()ing the results?

Quoted 5 lines

That's a LOT of stack space, which risks stack overflow, will mostly be unused, and still doesn't work if you have super-deep hierarchies larger than PATH_MAX. Would you be better off using realpath(,NULL) for its allocating semantics, and then free()ing the results?

That is the main reason I changed it from being a recursive function, to an iterative one.

Do we know that realpath(,NULL) behaves the same on all platforms?

We had a thread back in April that touched on the use of realpath, and concerns were raised then that realpath(,NULL) was not necessarily safe across all OSes:

https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01417.html

That said, if there is concern over the stack usage, to be safe I can manually g_malloc() each array.

Quoted 10 lines · Jeff Cody

Quoted 5 lines

That's a LOT of stack space, which risks stack overflow, will mostly be unused, and still doesn't work if you have super-deep hierarchies larger than PATH_MAX. Would you be better off using realpath(,NULL) for its allocating semantics, and then free()ing the results?

That is the main reason I changed it from being a recursive function, to an iterative one.

Do we know that realpath(,NULL) behaves the same on all platforms?

Gnulib lists the following platforms as mis-handling NULL:
Mac OS X 10.5, FreeBSD 6.4, OpenBSD 4.4, Solaris 10.

Quoted 6 lines

We had a thread back in April that touched on the use of realpath, and concerns were raised then that realpath(,NULL) was not necessarily safe across all OSes:

https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01417.html

In fact, that message points out an even more insidious portability bug in your algorithm: on Solaris 10, realpath("relative", buffer) leaves buffer containing "relative" rather than an absolute name, but your algorithm depends on matching absolute names. I don't know if we port qemu to Solaris 10, but it's worth considering my question back in that thread - does glib provide us a more portable function for converting a relative name into a canonical path that is guaranteed to work everywhere?

Quoted 3 lines

That said, if there is concern over the stack usage, to be safe I can manually g_malloc() each array.

g_malloc() would solve the stack size concern, but not the Solaris 10 relative name bug.

We are already relying on realpath() in block.c. So rather than making this patch series much more complicated for a minor fix, I think that if there are concerns about realpath() on other OSes such as Solaris 10, then those concerns should be addressed in a separate patch series.

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

Quoted 18 lines · Jeff Cody

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3146

Quoted 5 lines

block.c +5 -0

+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
+        /* If not an absolute filename path, make it relative to the current
+         * image's filename path */
+        path_combine(filename_tmp, sizeof(filename_tmp),
+                     curr_bs->filename, backing_file);

I just realized that it is possible to set up a qcow2 file that wraps a network protocol as its backing source, such as 'nbd:...'. In this case, what does path_combine() do to that user string?

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3151

Quoted 5 lines

block.c +5 -0

+
+        /* We are going to compare absolute pathnames */
+        if (!realpath(filename_tmp, filename_full)) {
+            continue;
+        }

and realpath() certainly won't like it (most likely, it won't exist in the file system, but on the off chance that it does, that file is much different than the real protocol that we are using as the backing source).

I'm afraid you may need a followup patch that handles the case of a non-file backing protocol, and insist on an exact match in that case without trying any normalization.

Quoted 39 lines · Jeff Cody

Quoted 18 lines

Currently, bdrv_find_backing_image compares bs->backing_file with what is passed in as a backing_file name. Mismatches may occur, however, when bs->backing_file and backing_file are both not absolute or relative.

Use path_combine() to make sure any relative backing filenames are relative to the current image filename being searched, and then use realpath() to make all comparisons based on absolute filenames.

This also changes bdrv_find_backing_image to no longer be recursive, but iterative.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3146

Quoted 5 lines

block.c +5 -0

+    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
+        /* If not an absolute filename path, make it relative to the current
+         * image's filename path */
+        path_combine(filename_tmp, sizeof(filename_tmp),
+                     curr_bs->filename, backing_file);

I just realized that it is possible to set up a qcow2 file that wraps a network protocol as its backing source, such as 'nbd:...'. In this case, what does path_combine() do to that user string?

block.c int bdrv_snapshot_load_tmp(BlockDriverState *bs, · line 3151

Quoted 5 lines

block.c +5 -0

+
+        /* We are going to compare absolute pathnames */
+        if (!realpath(filename_tmp, filename_full)) {
+            continue;
+        }

and realpath() certainly won't like it (most likely, it won't exist in the file system, but on the off chance that it does, that file is much different than the real protocol that we are using as the backing source).

Quoted 4 lines

I'm afraid you may need a followup patch that handles the case of a non-file backing protocol, and insist on an exact match in that case without trying any normalization.

Yes, it will need a follow-up patch. Check for protocol (path_has_protocol()), and if true just compare unmodified backing_file names, other wise use path_combine + realpath().

This simplifies some code and error checking, and also fixes a bug.

bdrv_find_backing_image() should only be passed absolute filenames, or filenames relative to the chain. In the QMP message handler for block commit, when looking up the base do so from the determined top image, so we know it is reachable from top.

Signed-off-by Jeff Cody <[email protected]>

---
 block/commit.c |  9 ---------
 blockdev.c     | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 19 deletions(-)

block/commit.c +0 -9

index 733c914..13d9e82 100644
@@ -211,15 +211,6 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
-    /* top and base may be valid, but let's make sure that base is reachable
-     * from top */
-    if (bdrv_find_backing_image(top, base->filename) != base) {
-        error_setg(errp,
-                   "Base (%s) is not reachable from top (%s)",
-                   base->filename, top->filename);
-        return;
-    }
-
     overlay_bs = bdrv_find_overlay(bs, top);
 
     if (overlay_bs == NULL) {

blockdev.c +4 -10

index 99828ad..7052287 100644
@@ -1157,16 +1157,6 @@ void qmp_block_commit(const char *device,
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
-    if (base && has_base) {
-        base_bs = bdrv_find_backing_image(bs, base);
-    } else {
-        base_bs = bdrv_find_base(bs);
-    }
-
-    if (base_bs == NULL) {
-        error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL");
-        return;
-    }
 
     /* default top_bs is the active layer */
     top_bs = bs;

@@ -1182,6 +1172,17 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    if (base && has_base) {
+        base_bs = bdrv_find_backing_image(top_bs, base);
+    } else {
+        base_bs = bdrv_find_base(top_bs);

This simplifies some code and error checking, and also fixes a bug.

bdrv_find_backing_image() should only be passed absolute filenames, or filenames relative to the chain. In the QMP message handler for block commit, when looking up the base do so from the determined top image, so we know it is reachable from top.

Signed-off-by Jeff Cody <[email protected]>

---
 block/commit.c |  9 ---------
 blockdev.c     | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 19 deletions(-)

This simplifies some code and error checking, and also fixes a bug.

bdrv_find_backing_image() should only be passed absolute filenames, or filenames relative to the chain. In the QMP message handler for block commit, when looking up the base do so from the determined top image, so we know it is reachable from top.

Signed-off-by Jeff Cody <[email protected]>

---

+++ b/blockdev.c