[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 04/28] 9pfs: introduce openat_nofollow() help
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 04/28] 9pfs: introduce openat_nofollow() helper |
Date: |
Mon, 27 Feb 2017 17:28:33 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
On 02/26/2017 04:42 PM, Greg Kurz wrote:
> When using the passthrough security mode, symbolic links created by the
> guest are actual symbolic links on the host file system.
>
>
> diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> new file mode 100644
> index 000000000000..62fd7a76212a
> --- /dev/null
> +int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode)
> +{
> + int fd;
> +
> + fd = dup(dirfd);
> + if (fd == -1) {
> + return -1;
> + }
> +
Do you want to assert that the caller's path does not start with '/'?
This function ignores dirfd in that case, which may not be what you want.
> + while (*path) {
> + const char *c;
> + int next_fd;
> + char *head;
> +
> + head = g_strdup(path);
> + c = strchr(path, '/');
So if the caller passes path="a//b", then the first iteration sets
head="a", but the second iteration sets head="".
> + if (c) {
> + head[c - path] = 0;
> + next_fd = openat_dir(fd, head);
The second iteration will then fail (openat_dir on "" should fail with
ENOENT, right?). Oops.
> + } else {
> + next_fd = openat_file(fd, head, flags, mode);
> + }
> + g_free(head);
> + if (next_fd == -1) {
> + close_preserve_errno(fd);
> + return -1;
> + }
> + close(fd);
> + fd = next_fd;
> +
> + if (!c) {
> + break;
> + }
> + path = c + 1;
I think the fix is that you should skip past all consecutive '/' here,
rather than assuming there is just one. Or can you assert that all
callers are well-behaved, and that *path is not '/' at this point?
> + }
> +static inline int openat_file(int dirfd, const char *name, int flags,
> + mode_t mode)
> +{
> + int fd, serrno;
> +
> + fd = openat(dirfd, name, flags | O_NOFOLLOW | O_NOCTTY | O_NONBLOCK,
> + mode);
> + if (fd == -1) {
> + return -1;
> + }
> +
> + serrno = errno;
> + /* O_NONBLOCK was only needed to open the file. Let's drop it. */
> + assert(!fcntl(fd, F_SETFL, flags));
Ouch - side effect inside an assertion. We don't support use of NDEBUG,
but this is poor practice.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 00/28] Series short description, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 05/28] 9pfs: local: keep a file descriptor on the shared folder, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 06/28] 9pfs: local: open/opendir: don't follow symlinks, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 07/28] 9pfs: local: lgetxattr: don't follow symlinks, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 08/28] 9pfs: local: llistxattr: don't follow symlinks, Greg Kurz, 2017/02/26
- [Qemu-devel] [PATCH v2 09/28] 9pfs: local: lsetxattr: don't follow symlinks, Greg Kurz, 2017/02/26