On Mon, 27 Feb 2017 17:28:33 -0600 Eric Blake <ebl...@redhat.com> wrote:
> 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 '/'?
Yes, I've added this for the pull request.
> This function ignores dirfd in that case, which may not be what you want.
>
Indeed, it really needs the path to be relative.
> > + 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="".
>
This doesn't happen with the current code, but you're right, we should
assert here also. We only wany a/b/c/d
>
> > + 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?
>
Again you're right :-\
> > + }
>
> > +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.
>
And I now remember you already made a similar comment in the past... I hope
I will remember this time.
Thanks!
pgpNqUMQxhZ_L.pgp
Description: OpenPGP digital signature
