Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody <jcody@redhat.com>]

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.