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.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)