[libvirt] [PATCHv2 2/5] conf: fix detection of infinite backing loop

[Peter Krempa]

On 04/08/14 06:07, Eric Blake wrote:
> While trying to refactor the backing file chain, I noticed that
> if you have a self-referential qcow2 file via a relative name:
> 
> qemu-img create -f qcow2 loop 10M
> qemu-img rebase -u -f qcow2 -F qcow2 -b loop loop
> 
> then libvirt was creating a chain 2 deep before realizing it
> had hit a loop; furthermore, virStorageFileChainCheckBroken
> was not identifying the chain as broken.  With this patch,
> the loop is detected when the chain is only 1 deep; still
> enough for storage volume XML to display the file, but now
> with a proper error report about where the loop was found.
> 
> * src/util/virstoragefile.c (virStorageFileGetMetadataRecurse):
> Add parameter, require canonical path up front.  Mark chain
> broken on OOM or loop detection.
> (virStorageFileGetMetadata): Pass in canonical name.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> v2: hoist canonical computation out of virStorageFileGetMetadataRecurse
> rest of patch series still works as is
> 
>  src/util/virstoragefile.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 3cdc89c..413ea5b 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1065,26 +1065,27 @@ virStorageFileGetMetadataFromFD(const char *path,
> 
>  /* Recursive workhorse for virStorageFileGetMetadata.  */
>  static virStorageFileMetadataPtr
> -virStorageFileGetMetadataRecurse(const char *path, const char *directory,
> +virStorageFileGetMetadataRecurse(const char *path, const char *canonPath,

Hmm, I think that passing in canonical path isn't necessary here. The
path argument is from the second iteration canonicalized by the metadata
retrieval function. So to avoid the bug you IMO just need to
canonicalize the first call of this function. It's true that it would
actually change the path as reported by debug/error messages but I don't
think that should be problematic.

> +                                 const char *directory,
>                                   int format, uid_t uid, gid_t gid,
>                                   bool allow_probe, virHashTablePtr cycle)
>  {
>      int fd;
> -    VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d",
> -              path, format, (int)uid, (int)gid, allow_probe);
> +    VIR_DEBUG("path=%s canonPath=%s format=%d uid=%d gid=%d probe=%d",
> +              path, canonPath, format, (int)uid, (int)gid, allow_probe);
> 
>      virStorageFileMetadataPtr ret = NULL;
> 
> -    if (virHashLookup(cycle, path)) {
> +    if (virHashLookup(cycle, canonPath)) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("backing store for %s is self-referential"),
>                         path);
>          return NULL;
>      }
> -    if (virHashAddEntry(cycle, path, (void *)1) < 0)
> +    if (virHashAddEntry(cycle, canonPath, (void *)1) < 0)
>          return NULL;
> 
> -    if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
> +    if ((fd = virFileOpenAs(canonPath, O_RDONLY, 0, uid, gid, 0)) < 0) {
>          virReportSystemError(-fd, _("Failed to open file '%s'"), path);
>          return NULL;
>      }
> @@ -1100,14 +1101,19 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory,
>          else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE)
>              ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO;
>          format = ret->backingStoreFormat;
> -        ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStore,
> +        ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStoreRaw,
> +                                                            ret->backingStore,
>                                                              ret->directory,
>                                                              format,
>                                                              uid, gid,
>                                                              allow_probe,
>                                                              cycle);
> +        if (!ret->backingMeta) {
> +            /* If we failed to get backing data, mark the chain broken */
> +            ret->backingStoreFormat = VIR_STORAGE_FILE_NONE;
> +            VIR_FREE(ret->backingStore);
> +        }
>      }
> -
>      return ret;
>  }
> 
> @@ -1142,15 +1148,23 @@ virStorageFileGetMetadata(const char *path, int format,
>                path, format, (int)uid, (int)gid, allow_probe);
> 
>      virHashTablePtr cycle = virHashCreate(5, NULL);
> -    virStorageFileMetadataPtr ret;
> +    virStorageFileMetadataPtr ret = NULL;
> +    char *canonPath;
> 
>      if (!cycle)
>          return NULL;
> 
> +    if (!(canonPath = canonicalize_file_name(path))) {
> +        virReportSystemError(errno, _("unable to resolve '%s'"), path);
> +        goto cleanup;
> +    }
> +
>      if (format <= VIR_STORAGE_FILE_NONE)
>          format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
> -    ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid,
> -                                           allow_probe, cycle);
> +    ret = virStorageFileGetMetadataRecurse(path, canonPath, NULL, format,

... Here I'd change path for canonPath and would not refactor the rest.

> +                                           uid, gid, allow_probe, cycle);
> + cleanup:
> +    VIR_FREE(canonPath);
>      virHashFree(cycle);
>      return ret;
>  }


Is there any reason you chose to add the parameter?

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140408/8593d181/attachment-0001.sig>