On 25.11.2013 22:49, Eric Blake wrote:
> Commit 348b4e2 introduced a potential problem (thankfully not
> in any release): we are attempting to use virFileReadHeaderFD()
> on a file that was opened with O_NONBLOCK. While this
> shouldn't be a problem in practice (because O_NONBLOCK
> typically doesn't affect regular or block files, and fifos and
> sockets cannot be storage volumes), it's better to play it safe
> to avoid races from opening an unexpected file type while also
> avoiding problems with having to handle EAGAIN while read()ing.
>
> Based on a report by Dan Berrange.
>
> * src/storage/storage_backend.c
> (virStorageBackendVolOpenCheckMode): Fix up fd after avoiding race.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/storage/storage_backend.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 57c1728..bde39d6 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1179,6 +1179,12 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
> return -2;
> }
>
> + /* O_NONBLOCK should only matter during open() for fifos and
> + * sockets, which we already filtered; but using it prevents a
> + * TOCTTOU race. However, later on we will want to read() the
> + * header from this fd, and virFileRead* routines require a
> + * blocking fd, so fix it up after verifying we avoided a
> + * race. */
> if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
> if ((errno == ENOENT || errno == ELOOP) &&
> S_ISLNK(sb->st_mode)) {
> @@ -1200,13 +1206,13 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
> return -1;
> }
>
> - if (S_ISREG(sb->st_mode))
> + if (S_ISREG(sb->st_mode)) {
> mode = VIR_STORAGE_VOL_OPEN_REG;
> - else if (S_ISCHR(sb->st_mode))
> + } else if (S_ISCHR(sb->st_mode)) {
> mode = VIR_STORAGE_VOL_OPEN_CHAR;
> - else if (S_ISBLK(sb->st_mode))
> + } else if (S_ISBLK(sb->st_mode)) {
> mode = VIR_STORAGE_VOL_OPEN_BLOCK;
> - else if (S_ISDIR(sb->st_mode)) {
> + } else if (S_ISDIR(sb->st_mode)) {
> mode = VIR_STORAGE_VOL_OPEN_DIR;
>
> if (STREQ(base, ".") ||
> @@ -1215,6 +1221,17 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
> VIR_INFO("Skipping special dir '%s'", base);
> return -2;
> }
> + } else {
> + VIR_WARN("ignoring unexpected type for file '%s'", path);
> + VIR_FORCE_CLOSE(fd);
> + return -2;
> + }
> +
> + if (virSetBlocking(fd, true) < 0) {
> + virReportSystemError(errno, _("unable to set blocking mode for '%s'"),
> + path);
> + VIR_FORCE_CLOSE(fd);
> + return -2;
> }
>
> if (!(mode & flags)) {
>
ACK
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list