Re: [PATCH v4 2/6] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Akihiko Odaki]

On 2022/04/28 20:22, Christian Schoenebeck wrote:
> On Mittwoch, 27. April 2022 22:36:25 CEST Greg Kurz wrote:
> > On Wed, 27 Apr 2022 20:54:17 +0200
> >
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > mknod() on macOS does not support creating sockets, so divert to
> > > call sequence socket(), bind() and fchmodat() respectively if S_IFSOCK
> > > was passed with mode argument.
> > >
> > > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > >
> > >   hw/9pfs/9p-util-darwin.c | 45 +++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > > index e24d09763a..7d00db47a9 100644
> > > --- a/hw/9pfs/9p-util-darwin.c
> > > +++ b/hw/9pfs/9p-util-darwin.c
> > > @@ -74,6 +74,45 @@ int fsetxattrat_nofollow(int dirfd, const char
> > > *filename, const char *name,>
> > >    */
> > >   
> > >   #if defined CONFIG_PTHREAD_FCHDIR_NP
> > >
> > > +static int create_socket_file_at_cwd(const char *filename, mode_t mode) {
> > > +    int fd, err;
> > > +    struct sockaddr_un addr = {
> > > +        .sun_family = AF_UNIX
> > > +    };
> > > +
> > > +    /*
> > > +     * sun_path is only 104 bytes, explicit filename length check
> > > required
> > > +     */
> > > +    if (sizeof(addr.sun_path) - 1 < strlen(filename) + 2) {
> >
> > True but I was a bit puzzled by the math until I realized the '+ 2' was
> > for the prepended "./" ;-)
>
> Correct ...
>
> > > +        errno = ENAMETOOLONG;
> > > +        return -1;
> > > +    }
> > > +    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
> > > +    if (fd == -1) {
> > > +        return fd;
> > > +    }
> > > +    snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename);
> >
> > What about the more generic approach of checking snprintf()'s return
> > value ? If it is >= sizeof(addr.sun_path) then truncation occured.
>
> ... well, I can send a v5 if you prefer that solution, or you can send a 
> follow-up
> patch later on. As you wish.

I also prefer that solution and would like to see v5. The checking code 
has a redundant knowledge about how the output length becomes, and it is 
dangerous if it becomes out of sync with the snprintf() call. Using the 
value returned by snprintf() would eliminate such a potential 
programming error in the future.

> > > +    err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
> > > +    if (err == -1) {
> > > +        goto out;
> > > +    }
> > > +    /*
> > > +     * FIXME: Should rather be using descriptor-based fchmod() on the
> > > +     * socket file descriptor above (preferably before bind() call),
> > > +     * instead of path-based fchmodat(), to prevent concurrent transient
> > > +     * state issues between creating the named FIFO file at bind() and
> > > +     * delayed adjustment of permissions at fchmodat(). However currently
> > > +     * macOS (12.x) does not support such operations on socket file
> > > +     * descriptors yet.
> > > +     *
> > > +     * Filed report with Apple: FB9997731
> > > +     */
> > > +    err = fchmodat(AT_FDCWD, filename, mode, AT_SYMLINK_NOFOLLOW_ANY);
> > > +out:
> > > +    close_preserve_errno(fd);
> >
> > You could close(fd) earlier now, but you might want to keep the code
> > as is in case FB9997731 gets proper attention.
> >
> > Anyway, this should do the job so:
>
> Sounds like Akihiko's previous suggestion. I would keep it that way:
> eafd4bbf-dbff-323a-179f-8f29905701e1@gmail.com/">https://lore.kernel.org/qemu-devel/eafd4bbf-dbff-323a-179f-8f29905701e1@gmail.com/
I don't think it would need an extra code change when FB9997731 is 
resolved even if you close the socket immediately after bind(). However, 
I'm not pursuing such a change since doing so would have the same 
trade-off my previous suggestion as Christian noted.

Regards,
Akihiko Odaki

> > Reviewed-by: Greg Kurz <groug@kaod.org>
>
> Thanks!
>
> Best regards,
> Christian Schoenebeck
>
> > > +    return err;
> > > +}
> > > +
> > >
> > >   int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> > >   {
> > >   
> > >       int preserved_errno, err;
> > >
> > > @@ -93,7 +132,11 @@ int qemu_mknodat(int dirfd, const char *filename,
> > > mode_t mode, dev_t dev)>
> > >       if (pthread_fchdir_np(dirfd) < 0) {
> > >       
> > >           return -1;
> > >       
> > >       }
> > >
> > > -    err = mknod(filename, mode, dev);
> > > +    if (S_ISSOCK(mode)) {
> > > +        err = create_socket_file_at_cwd(filename, mode);
> > > +    } else {
> > > +        err = mknod(filename, mode, dev);
> > > +    }
> > >
> > >       preserved_errno = errno;
> > >       /* Stop using the thread-local cwd */
> > >       pthread_fchdir_np(-1);