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

[Christian Schoenebeck]

On Freitag, 29. April 2022 14:56:50 CEST Greg Kurz wrote:
> On Fri, 29 Apr 2022 12:25:11 +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 | 42 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > index e24d09763a..619c403ba7 100644
> > --- a/hw/9pfs/9p-util-darwin.c
> > +++ b/hw/9pfs/9p-util-darwin.c
> > @@ -74,6 +74,42 @@ 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
> > +    };
> > +
> > +    err = snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s",
> > filename); +    if (err < 0 || err >= sizeof(addr.sun_path)) {
> 
> According to POSIX [1]:
> 
> The snprintf() function shall fail if:
> 
> [EOVERFLOW]
> [CX] [Option Start] The value of n is greater than {INT_MAX}. [Option End]
> 
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html
> 
> Since we're passing sizeof(addr.sun_path), I'm pretty sure snprintf()
> cannot fail. No big deal.

The question is whom you would want to trust on this? POSIX? ISO-C? Clang? 
BSD? Apple? And for how long into future? I mean in general yes, I would not 
expect it to fail with -1 here either, but there are various different API 
docs on snprintf() out there, and most of them don't even bother to enumarate 
which encoding errors may happen. And I'm pretty sure if I'd drop the negative 
err check here, then Akihiko would slap me for unforeseeable additional error 
cases on snprintf() that may be added in future.

Apple's documentation on snprintf() BTW just says:

  "These functions return a negative value if an error occurs."

So Apple does not even restrict the return value to -1 on errrors, you would 
also need to expect other negative values.

So on doubt, I leave this negative result check for now. ;-)

> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks!

Best regards
Christian Schoenebeck

> > +        errno = ENAMETOOLONG;
> > +        return -1;
> > +    }
> > +    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
> > +    if (fd == -1) {
> > +        return fd;
> > +    }
> > +    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);
> > +    return err;
> > +}
> > +
> > 
> >  int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
> >  {
> >  
> >      int preserved_errno, err;
> > 
> > @@ -93,7 +129,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);