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

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.