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

On 05/20/2010 01:23 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 a single function which runs stat() before any open() call. Directory > pools can then proceed along, ignoring the invalid files. stat() before open() is racy. Better yet is using the O_NONBLOCK|O_NOCTTY flags of open(); gnulib guarantees they are defined, and I recently lobbied POSIX to guarantee that it will be safe on all platforms (it is already safe on Linux): http://austingroupbugs.net/view.php?id=141 > > +int > +virStorageBackendVolOpen(const char *path) > +{ > + int fd; > + struct stat sb; > + > + if (stat(path, &sb) < 0) { > + virReportSystemError(errno, > + _("cannot stat file '%s'"), path); > + return -1; > + } > + > + if (!S_ISREG(sb.st_mode) && > + !S_ISCHR(sb.st_mode) && > + !S_ISBLK(sb.st_mode)) { Regular files and block devices I can understand, but character devices? That includes things like /dev/zero and /dev/null; do those really make sense as the backing of a volume store?

> + VIR_DEBUG("Skipping volume path %s, unexpected file mode.", path); > + return -2; > + } > + > + if ((fd = open(path, O_RDONLY)) < 0) { > + virReportSystemError(errno, > + _("cannot open volume '%s'"), > + path); In other words, I'd rather see open(path,O_RDONLY|O_NONBLOCK|O_NOCTTY) then fstat(), and not your stat()/open() sequence. But the rest of the patch looks sane.