Patchwork [1/2] block: make bdrv_find_backing_image compare canonical filenames

login
register
mail settings
Submitter Jeff Cody
Date Oct. 10, 2012, 5:56 a.m.
Message ID <37ca1a150224c144f9329fc8408f78c388f50a02.1349848348.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/190548/
State New
Headers show

Comments

Jeff Cody - Oct. 10, 2012, 5:56 a.m.
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 <jcody@redhat.com>
---
 block.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)
Paolo Bonzini - Oct. 10, 2012, 7:34 a.m.
Il 10/10/2012 07:56, Jeff Cody ha scritto:
> 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 <jcody@redhat.com>
> ---
>  block.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e95f613..641b8fa 100644
> --- a/block.c
> +++ b/block.c
> @@ -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 */
> +        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

> +            continue;
> +        }
> +
> +        if (strcmp(backing_file_full, filename_full) == 0) {
> +            return curr_bs->backing_hd;
>          }
>      }
>  
>
Jeff Cody - Oct. 10, 2012, 7:59 a.m.
On 10/10/2012 03:34 AM, Paolo Bonzini wrote:
> Il 10/10/2012 07:56, Jeff Cody ha scritto:
>> 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 <jcody@redhat.com>
>> ---
>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e95f613..641b8fa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -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 */
>> +        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
> 

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

>> +            continue;
>> +        }
>> +
>> +        if (strcmp(backing_file_full, filename_full) == 0) {
>> +            return curr_bs->backing_hd;
>>          }
>>      }
>>  
>>
>
Paolo Bonzini - Oct. 10, 2012, 8:07 a.m.
Il 10/10/2012 09:59, Jeff Cody ha scritto:
>> 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?

Uh, indeed there's already a _fullpath wrapper in qemu-common.h.  My
bad, I only grepped os*.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo
Eric Blake - Oct. 10, 2012, 6:13 p.m.
On 10/09/2012 11:56 PM, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
>  block.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e95f613..641b8fa 100644
> --- a/block.c
> +++ b/block.c
> @@ -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];

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?
Jeff Cody - Oct. 10, 2012, 6:29 p.m.
On 10/10/2012 02:13 PM, Eric Blake wrote:
> On 10/09/2012 11:56 PM, Jeff Cody wrote:
>> 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 <jcody@redhat.com>
>> ---
>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e95f613..641b8fa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -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];
> 
> 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.
Eric Blake - Oct. 10, 2012, 6:34 p.m.
On 10/10/2012 12:29 PM, Jeff Cody wrote:

>> 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.

> 
> 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?

> 
> 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.
Jeff Cody - Oct. 10, 2012, 6:57 p.m.
On 10/10/2012 02:34 PM, Eric Blake wrote:
> On 10/10/2012 12:29 PM, Jeff Cody wrote:
> 
>>> 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.
>
>>
>> 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?
> 

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.

>>
>> 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.
>
Eric Blake - Oct. 10, 2012, 7:15 p.m.
On 10/10/2012 12:57 PM, Jeff Cody wrote:
> 
> 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.

I can agree to that.  Therefore,

Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake - Oct. 12, 2012, 9:52 p.m.
On 10/09/2012 11:56 PM, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
>  block.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 

> +    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?

> +
> +        /* 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.
Jeff Cody - Oct. 13, 2012, 3:25 p.m.
On 10/12/2012 05:52 PM, Eric Blake wrote:
> On 10/09/2012 11:56 PM, Jeff Cody wrote:
>> 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 <jcody@redhat.com>
>> ---
>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
> 
>> +    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?
> 
>> +
>> +        /* 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.
> 

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().
Kevin Wolf - Oct. 16, 2012, 11:40 a.m.
Am 13.10.2012 17:25, schrieb Jeff Cody:
> On 10/12/2012 05:52 PM, Eric Blake wrote:
>> On 10/09/2012 11:56 PM, Jeff Cody wrote:
>>> 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 <jcody@redhat.com>
>>> ---
>>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>>
>>
>>> +    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?
>>
>>> +
>>> +        /* 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.
>>
> 
> 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().

These cases would regress, so it doesn't just need a follow-up patch,
but a respin of this series.

Kevin

Patch

diff --git a/block.c b/block.c
index e95f613..641b8fa 100644
--- a/block.c
+++ b/block.c
@@ -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 */
+        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)) {
+            continue;
+        }
+
+        if (strcmp(backing_file_full, filename_full) == 0) {
+            return curr_bs->backing_hd;
         }
     }