Re: [PATCH v3] storage: Check for invalid storage mode before opening

[Eric Blake <eblake@xxxxxxxxxx>]

On 05/24/2010 03:36 PM, Cole Robinson wrote:
> If a directory pool contains pipes or sockets, a pool start can fail or hang:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=589577
> 
> We already try to avoid these special files, but only attempt after
> opening the path, which is where the problems lie. Unify volume opening
> into helper functions, which use the proper open() flags to avoid error,
> followed by fstat to validate storage mode.
> 
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -872,6 +872,61 @@ virStorageBackendForType(int type) {
>      return NULL;
>  }
>  
> +static int
> +virStorageBackendVolOpenInternal(const char *path)
> +{
> +    int fd;
> +    struct stat sb;
> +
> +    if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot open volume '%s'"),
> +                             path);
> +        return -1;
> +    }
> +
> +    if (fstat(fd, &sb) < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot stat file '%s'"),
> +                             path);
> +        close(fd);
> +        return -1;
> +    }
> +
> +    if (!S_ISREG(sb.st_mode) &&
> +        !S_ISCHR(sb.st_mode) &&
> +        !S_ISBLK(sb.st_mode)) {
> +        close(fd);
> +        return -2;
> +    }
> +
> +    return fd;
> +}
> +
> +/*
> + * Allows caller to silently ignore files with improper mode
> + *
> + * Returns -1 on error, -2 if file mode is unexpected.
> + */
> +int virStorageBackendVolOpenModeSkip(const char *path)
> +{
> +    return virStorageBackendVolOpenInternal(path);
> +}
> +
> +int
> +virStorageBackendVolOpen(const char *path)

Hmm, I'm thinking that better semantics might be:

enum {
  VIR_STORAGE_OPEN_WARN = 1 << 0; // warn if unexpected type encountered
  VIR_STORAGE_OPEN_REG = 1 << 1; // regular files okay
  VIR_STORAGE_OPEN_BLOCK = 1 << 2; // block files okay
  VIR_STORAGE_OPEN_DIR = 1 << 3; // directories okay
  ...
}

int virStorageBackendVolOpen(const char *path) {
    return virStorageBackendVolOpenCheckMode(path, 0);
}

int virStorageBackendVolOpenCheckMode(const char *path,
      unsigned int flags) {
  open
  fstat
  if (flags & VIR_STORAGE_OPEN_WARN) {
    if (type == REG && !(flags & VIR_STORAGE_OPEN_REG))
       warn
    else if (type == DIR && !(flags & VIR_STORAGE_OPEN_DIR))
       warn
    ...
  }
  return result;
}

Then, you can preserve existing semantics of no warning by calling the
one-argument version, or fine-tune which file types you are willing to
accept (some callers will accept directories, others will not) by
calling the two-arg version.

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list