Upon further review, it looks like since 10.12 there's actually a (not-heavily-documented) function that wraps this syscall and avoids the need to call the private syscall directly: https://opensource.apple.com/source/libpthread/libpthread-218.51.1/src/pthread_cwd.c.auto.html. Chromium uses it too ( https://chromium.googlesource.com/chromium/src/+/lkgr/base/process/launch_mac.cc#110) -- given that we're not looking for pre-10.12 compatibility, I'm a little less worried about the workaround breaking in the future if this wrapper gets used instead.
Would it work to change to pthread_fchdir_np, remove all the syscall discussion in the comment, and add a meson check for pthread_fchdir_np as a prereq for virtfs on darwin? On Fri, Jan 28, 2022 at 1:28 PM Will Cohen <wwco...@gmail.com> wrote: > Understood. Since I cannot find the original number, I have submitted a > new report at rdar://FB9862426 <https://openradar.appspot.com/FB9862426> ( > https://openradar.appspot.com/FB9862426). > > I'll note that and work on a testcase/error message for v4. > > Many thanks, > Will > > On Fri, Jan 28, 2022 at 10:15 AM Christian Schoenebeck < > qemu_...@crudebyte.com> wrote: > >> On Donnerstag, 27. Januar 2022 22:47:54 CET Will Cohen wrote: >> > Back when this was being proposed, the original proposer did file such a >> > report to Apple, but we're still in this situation! >> >> Ok, but still, do you find it appropriate to just blindly use a private >> syscall that may or may not exist or might change its behaviour at any >> time >> without a user being aware? >> >> I am not opposed to using workarounds at all, but what I worry about is >> that >> Apple might change this in whatever way at any time, and as this sycall >> is >> currently not guarded in this patch at all, we might one day receive bug >> reports by macOS users with symptoms that might not immediately be >> obvious to >> relate to this being the root cause. >> >> Options that would come to my mind: >> - a test case for this syscall >> - a clear runtime error message for ordinary users >> >> Is there a rdar or FB number for the report on Apple's side? >> >> > Replacing clang with gcc in v3. >> > >> > On Wed, Nov 24, 2021 at 12:20 PM Christian Schoenebeck < >> > >> > qemu_...@crudebyte.com> wrote: >> > > On Montag, 22. November 2021 01:49:12 CET Will Cohen wrote: >> > > > From: Keno Fischer <k...@juliacomputing.com> >> > > > >> > > > Darwin does not support mknodat. However, to avoid race conditions >> > > > with later setting the permissions, we must avoid using mknod on >> > > > the full path instead. We could try to fchdir, but that would cause >> > > > problems if multiple threads try to call mknodat at the same time. >> > > > However, luckily there is a solution: Darwin as an (unexposed in the >> > > > C library) system call that sets the cwd for the current thread >> only. >> > > > This should suffice to use mknod safely. >> > > > >> > > > Signed-off-by: Keno Fischer <k...@juliacomputing.com> >> > > > Signed-off-by: Michael Roitzsch <reactorcont...@icloud.com> >> > > > [Will Cohen: - Adjust coding style] >> > > > Signed-off-by: Will Cohen <wwco...@gmail.com> >> > > > --- >> > > > >> > > > hw/9pfs/9p-local.c | 5 +++-- >> > > > hw/9pfs/9p-util-darwin.c | 33 +++++++++++++++++++++++++++++++++ >> > > > hw/9pfs/9p-util-linux.c | 5 +++++ >> > > > hw/9pfs/9p-util.h | 2 ++ >> > > > 4 files changed, 43 insertions(+), 2 deletions(-) >> > > > >> > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >> > > > index 4268703d05..42b65e143b 100644 >> > > > --- a/hw/9pfs/9p-local.c >> > > > +++ b/hw/9pfs/9p-local.c >> > > > @@ -673,7 +673,7 @@ static int local_mknod(FsContext *fs_ctx, >> V9fsPath >> > > > *dir_path, >> > > > >> > > > if (fs_ctx->export_flags & V9FS_SM_MAPPED || >> > > > >> > > > fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) { >> > > > >> > > > - err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0); >> > > > + err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, >> 0); >> > > > >> > > > if (err == -1) { >> > > > >> > > > goto out; >> > > > >> > > > } >> > > > >> > > > @@ -688,7 +688,7 @@ static int local_mknod(FsContext *fs_ctx, >> V9fsPath >> > > > *dir_path, } >> > > > >> > > > } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH || >> > > > >> > > > fs_ctx->export_flags & V9FS_SM_NONE) { >> > > > >> > > > - err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev); >> > > > + err = qemu_mknodat(dirfd, name, credp->fc_mode, >> > > > credp->fc_rdev); >> > > > >> > > > if (err == -1) { >> > > > >> > > > goto out; >> > > > >> > > > } >> > > > >> > > > @@ -701,6 +701,7 @@ static int local_mknod(FsContext *fs_ctx, >> V9fsPath >> > > > *dir_path, >> > > > >> > > > err_end: >> > > > unlinkat_preserve_errno(dirfd, name, 0); >> > > > >> > > > + >> > > > >> > > > out: >> > > > close_preserve_errno(dirfd); >> > > > return err; >> > > > >> > > > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c >> > > > index ac414bcbfd..25e67d5067 100644 >> > > > --- a/hw/9pfs/9p-util-darwin.c >> > > > +++ b/hw/9pfs/9p-util-darwin.c >> > > > >> > > > @@ -158,3 +158,36 @@ done: >> > > > close_preserve_errno(fd); >> > > > return ret; >> > > > >> > > > } >> > > > >> > > > + >> > > > +#ifndef SYS___pthread_fchdir >> > > > +# define SYS___pthread_fchdir 349 >> > > > +#endif >> > > > + >> > > > +/* >> > > > + * This is an undocumented OS X syscall. It would be best to avoid >> it, >> > > > + * but there doesn't seem to be another safe way to implement >> mknodat. >> > > > + * Dear Apple, please implement mknodat before you remove this >> syscall. >> > > > + */ >> > > > +static int fchdir_thread_local(int fd) >> > > >> > > Hooo, that's a brave move. Shouldn't its future and likely becoming >> > > absence be >> > > guarded "somehow"? :) >> > > >> > > BTW it might make sense to file a report instead of hoping Apple will >> just >> > > read this comment: ;-) >> > > https://feedbackassistant.apple.com/ >> > > >> > > > +{ >> > > > +#pragma clang diagnostic push >> > > > +#pragma clang diagnostic ignored "-Wdeprecated-declarations" >> > > > + return syscall(SYS___pthread_fchdir, fd); >> > > > +#pragma clang diagnostic pop >> > > > +} >> > > >> > > Consider s/clang/GCC/ then it would also work with GCC. In the end >> most >> > > people >> > > probably just use clang on macOS anyway, but just saying. >> > > >> > > > + >> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, >> dev_t >> > > >> > > dev) >> > > >> > > > +{ >> > > > + int preserved_errno, err; >> > > > + if (fchdir_thread_local(dirfd) < 0) { >> > > > + return -1; >> > > > + } >> > > > + err = mknod(filename, mode, dev); >> > > > + preserved_errno = errno; >> > > > + /* Stop using the thread-local cwd */ >> > > > + fchdir_thread_local(-1); >> > > > + if (err < 0) { >> > > > + errno = preserved_errno; >> > > > + } >> > > > + return err; >> > > > +} >> > > > diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c >> > > > index d54bf57a59..4f57d8c047 100644 >> > > > --- a/hw/9pfs/9p-util-linux.c >> > > > +++ b/hw/9pfs/9p-util-linux.c >> > > > @@ -68,3 +68,8 @@ int utimensat_nofollow(int dirfd, const char >> > > > *filename, >> > > > >> > > > { >> > > > >> > > > return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW); >> > > > >> > > > } >> > > > >> > > > + >> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, >> dev_t >> > > >> > > dev) >> > > >> > > > +{ >> > > > + return mknodat(dirfd, filename, mode, dev); >> > > > +} >> > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h >> > > > index 1c477a0e66..cac682d335 100644 >> > > > --- a/hw/9pfs/9p-util.h >> > > > +++ b/hw/9pfs/9p-util.h >> > > > @@ -105,4 +105,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const >> > > > char >> > > > *filename, int utimensat_nofollow(int dirfd, const char *filename, >> > > > >> > > > const struct timespec times[2]); >> > > > >> > > > +int qemu_mknodat(int dirfd, const char *filename, mode_t mode, >> dev_t >> > > >> > > dev); >> > > >> > > > + >> > > > >> > > > #endif >> >> >> Best regards, >> Christian Schoenebeck >> >> >>
