| Message ID | 37ca1a150224c144f9329fc8408f78c388f50a02.1349848348.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; > } > } > >
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; >> } >> } >> >> >
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
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?
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.
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.
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. >
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>
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.
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().
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
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; } }
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(-)