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

[Daniel P. Berrange]

On Thu, May 20, 2010 at 04:25:54PM -0600, Eric Blake wrote:
> 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?

For a directory based storage pool we don't want to include block devices
or character devices IMHO. Those are already dealt with via the other
types of storage pool. We *do* however want to allow directories, since
this allows for enumerating directories used a virtual root FS for 
container based virt. So this check should be

   !S_ISREG && S_ISDIR


NB, we will of course need to make sure later code handles directories
properly - ie don't have a fatal error upon trying to seek() in a 
directory, or even better skip that bit of code.


> > +        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.


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|