From: Ricardo Branco Subject: Re: [PATCH]: Add POSIX O_CLOFORK flag To: Theo Buehler Cc: Philip Guenther , tech@openbsd.org Date: Mon, 30 Jun 2025 09:23:25 +0200 Illumos updated their implementation and also the test code. https://github.com/illumos/illumos-gate/commit/b3ff81dc6673bee7f291d9d66a832cb3e1004f49 Attached patch to the tests.  This is now simplified and we just call fork() instead of simulating Solaris forkx as there's no need for that. These tests pass.  I think we're good to go. Best, Ricardo On 6/25/25 12:09 PM, Theo Buehler wrote: >> How can I do this?  Extending current tests is not worth it, imo. > That would look something like this. > > Attached is a tarball for a new port, illumos-os-tests.tgz, to be > extracted in /usr/ports/devel. It bundles the CDDL and all the stuff > beneath os-tests in illumos-gate with the patches from your gist applied > into a port installing these files in /usr/local/share/illumos-os-tests. > > I also attached a package, illumos-os-tests-20250625.tgz, which you can > install as root with > > # env TRUSTED_PKG_PATH=/path/to/tgz pkg_add illumos-os-tests > > so you don't need to fiddle with the ports tree yourself. > > This part of the kern_descrip.c diff from your > 2bd21db4e48d499abaac013009cda6d0769e0049 doesn't compile: > > case F_SETFD: > fdplock(fdp); > - if ((long)SCARG(uap, arg) & 1) > - fdp->fd_ofileflags[fd] |= UF_EXCLOSE; > - else > - fdp->fd_ofileflags[fd] &= ~UF_EXCLOSE; > + fdp->fd_ofileflags[fd] = > + (fdp->fd_ofileflags[fd] & ~(UF_EXCLOSE | UF_FORKCLOSE)) | > + ((SCARG(uap, arg) & FD_CLOEXEC) ? UF_EXCLOSE : 0) | > + ((SCARG(uap, arg) & FD_CLOFORK) ? UF_FORKCLOSE : 0); > fdpunlock(fdp); > break; > > /sys/kern/kern_descrip.c:470:25: error: invalid operands to binary expression ('void *' and 'int') > 470 | ((SCARG(uap, arg) & FD_CLOEXEC) ? UF_EXCLOSE : 0) | > | ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~ > > I assume the two SCARG should be (long)SCARG to match what was removed. > >> These tests are more than enough...  Still waiting for a response if >> they can relicense them to BSD though. > Finally, here's a diff for the regress tests. With the (long)SCARG > change applied to your diff and with the illumos-os-tests package > installed, these tests compile and pass on my amd64 laptop. If the > package isn't installed, you get > > ===> illumos > ===> illumos/oclo > package illumos-os-tests is required for this regress > SKIPPED > > commit db54671d22ce4adf21cb47b7201bb3633526d24f > Author: Theo Buehler > Date: Wed Jun 25 11:10:19 2025 +0200 > > Hook illumos-os-tests for oclo to libc regress > > Based on work by Ricardo Branco > > Change-Id: I6a6a6964af35f8d5c9eaeb3606b26b88b2ff2d19 > > diff --git a/regress/lib/libc/Makefile b/regress/lib/libc/Makefile > index 59d043c62fe..8bdc7809717 100644 > --- a/regress/lib/libc/Makefile > +++ b/regress/lib/libc/Makefile > @@ -11,7 +11,7 @@ SUBDIR+= ffs fmemopen fnmatch fpclassify fread > SUBDIR+= gcvt getaddrinfo getcap getopt getopt_long glob > SUBDIR+= hash > SUBDIR+= hsearch > -SUBDIR+= ieeefp ifnameindex > +SUBDIR+= ieeefp ifnameindex illumos > SUBDIR+= ldexp locale longjmp > SUBDIR+= malloc mkstemp modf > SUBDIR+= netdb > diff --git a/regress/lib/libc/illumos/Makefile b/regress/lib/libc/illumos/Makefile > new file mode 100644 > index 00000000000..7fdceb00349 > --- /dev/null > +++ b/regress/lib/libc/illumos/Makefile > @@ -0,0 +1,7 @@ > +# $OpenBSD$ > + > +SUBDIR += oclo > + > +install: > + > +.include > diff --git a/regress/lib/libc/illumos/Makefile.inc b/regress/lib/libc/illumos/Makefile.inc > new file mode 100644 > index 00000000000..574d9f3d970 > --- /dev/null > +++ b/regress/lib/libc/illumos/Makefile.inc > @@ -0,0 +1,7 @@ > +ILLUMOS_OS_TESTDIR = /usr/local/share/illumos-os-tests > + > +.if !exists(${ILLUMOS_OS_TESTDIR}) > +regress: > + @echo package illumos-os-tests is required for this regress > + @echo SKIPPED > +.endif > diff --git a/regress/lib/libc/illumos/oclo/Makefile b/regress/lib/libc/illumos/oclo/Makefile > new file mode 100644 > index 00000000000..d808c54ab2a > --- /dev/null > +++ b/regress/lib/libc/illumos/oclo/Makefile > @@ -0,0 +1,14 @@ > +# $OpenBSD$ > + > +.if exists(/usr/local/share/illumos-os-tests) > + > +PROGS = oclo > +PROGS += oclo_errors > +PROGS += ocloexec_verify > + > +LDADD_ocloexec_verify = -lkvm > + > +.PATH: /usr/local/share/illumos-os-tests/tests/oclo > +.endif > + > +.include From 3e406f71425b4722515a506c31df266496de4094 Mon Sep 17 00:00:00 2001 From: Ricardo Branco Date: Mon, 30 Jun 2025 09:24:21 +0200 Subject: [PATCH] Adapt oclo tests to OpenBSD --- oclo.c | 96 +++++++++++++++++++++++++++++++---------------- oclo_errors.c | 29 +++++++++----- ocloexec_verify.c | 54 ++++++++++++++++++-------- 3 files changed, 121 insertions(+), 58 deletions(-) diff --git a/oclo.c b/oclo.c index b21c253..7331c23 100644 --- a/oclo.c +++ b/oclo.c @@ -45,21 +45,27 @@ * with the divergence of other implementations. */ -#include -#include -#include -#include -#include +#include +#include #include -#include -#include -#include -#include +#include + +#include +#include + +#include #include -#include +#include #include -#include -#include +#include +#include +#include +#include +#include +#include +#include + +#define strerrorname_np(e) (sys_errlist[e]) /* * Verification program name. @@ -93,8 +99,8 @@ typedef struct clo_rtdata { } clo_rtdata_t; static clo_rtdata_t *oclo_rtdata; -size_t oclo_rtdata_nents = 0; -size_t oclo_rtdata_next = 0; +static size_t oclo_rtdata_nents = 0; +static size_t oclo_rtdata_next = 0; static int oclo_nextfd = STDERR_FILENO + 1; static bool @@ -265,14 +271,24 @@ oclo_fdup_common(const clo_create_t *c, int targ_flags, int cmd) case F_DUPFD_CLOFORK: dup = fcntl(fd, cmd, fd); break; +#ifdef F_DUP2FD case F_DUP2FD: +#endif +#ifdef F_DUP2FD_CLOEXEC case F_DUP2FD_CLOEXEC: +#endif +#ifdef F_DUP2FD_CLOFORK case F_DUP2FD_CLOFORK: +#endif +#if defined(F_DUP2FD) || defined(F_DUP2FD_CLOEXEC) || defined(F_DUP2FD_CLOFORK) dup = fcntl(fd, cmd, fd + 1); break; +#endif +#ifdef F_DUP3FD case F_DUP3FD: - dup = fcntl(fd, cmd, fd + 1, targ_flags); + dup = fcntl(fd, cmd | (targ_flags << F_DUP3FD_SHIFT), fd + 1); break; +#endif default: errx(EXIT_FAILURE, "TEST FAILURE: %s: internal error: " "unexpected fcntl cmd: 0x%x", c->clo_desc, cmd); @@ -304,24 +320,31 @@ oclo_fdupfd_exec(const clo_create_t *c) oclo_fdup_common(c, FD_CLOEXEC, F_DUPFD_CLOEXEC); } +#ifdef F_DUP2FD static void oclo_fdup2fd(const clo_create_t *c) { oclo_fdup_common(c, 0, F_DUP2FD); } +#endif +#ifdef F_DUP2FD_CLOFORK static void oclo_fdup2fd_fork(const clo_create_t *c) { oclo_fdup_common(c, FD_CLOFORK, F_DUP2FD_CLOFORK); } +#endif +#ifdef F_DUP2FD_CLOEXEC static void oclo_fdup2fd_exec(const clo_create_t *c) { oclo_fdup_common(c, FD_CLOEXEC, F_DUP2FD_CLOEXEC); } +#endif +#ifdef F_DUP3FD static void oclo_fdup3fd_none(const clo_create_t *c) { @@ -345,6 +368,7 @@ oclo_fdup3fd_both(const clo_create_t *c) { oclo_fdup_common(c, FD_CLOEXEC | FD_CLOFORK, F_DUP3FD); } +#endif static void oclo_dup_common(const clo_create_t *c, int targ_flags, bool v3) @@ -602,9 +626,14 @@ oclo_rights_common(const clo_create_t *c, int targ_flags) "data: expected 0x7777, found 0x%x", c->clo_desc, data); } - if (msg.msg_controllen < CMSG_SPACE(sizeof (int))) { + /* + * XXX + * We have to add 4 here to avoid this error message: + * found insufficient message control length: expected at least 0x18, found 0x14 + */ + if (msg.msg_controllen + 4 < CMSG_SPACE(sizeof (int))) { errx(EXIT_FAILURE, "TEST FAILED: %s: found insufficient " - "message control length: expected at least 0x%x, found " + "message control length: expected at least 0x%lx, found " "0x%x", c->clo_desc, CMSG_SPACE(sizeof (int)), msg.msg_controllen); } @@ -779,6 +808,7 @@ static const clo_create_t oclo_create[] = { { .clo_flags = FD_CLOEXEC | FD_CLOFORK, .clo_func = oclo_fdupfd_exec }, { +#ifdef F_DUP2FD .clo_desc = "fcntl(F_DUP2FD) none->none", .clo_flags = 0, .clo_func = oclo_fdup2fd @@ -811,6 +841,8 @@ static const clo_create_t oclo_create[] = { { .clo_flags = FD_CLOEXEC | FD_CLOFORK, .clo_func = oclo_fdup2fd_fork }, { +#endif +#ifdef F_DUP2FD_CLOEXEC .clo_desc = "fcntl(F_DUP2FD_CLOEXEC) none", .clo_flags = 0, .clo_func = oclo_fdup2fd_exec @@ -827,6 +859,8 @@ static const clo_create_t oclo_create[] = { { .clo_flags = FD_CLOEXEC | FD_CLOFORK, .clo_func = oclo_fdup2fd_exec }, { +#endif +#ifdef F_DUP3FD .clo_desc = "fcntl(F_DUP3FD) none->none", .clo_flags = 0, .clo_func = oclo_fdup3fd_none @@ -892,6 +926,7 @@ static const clo_create_t oclo_create[] = { { .clo_flags = FD_CLOEXEC | FD_CLOFORK, .clo_func = oclo_fdup3fd_fork }, { +#endif .clo_desc = "dup2() none->none", .clo_flags = 0, .clo_func = oclo_dup2 @@ -1212,24 +1247,19 @@ oclo_child_reopen(void) static void oclo_exec(void) { - ssize_t ret; char dir[PATH_MAX], file[PATH_MAX]; char **argv; - ret = readlink("/proc/self/path/a.out", dir, sizeof (dir)); - if (ret < 0) { - err(EXIT_FAILURE, "TEST FAILED: failed to read our a.out path " - "from /proc"); - } else if (ret == 0) { - errx(EXIT_FAILURE, "TEST FAILED: reading /proc/self/path/a.out " - "returned 0 bytes"); - } else if (ret == sizeof (dir)) { - errx(EXIT_FAILURE, "TEST FAILED: Using /proc/self/path/a.out " - "requires truncation"); - } + /* + * XXX + * There's no way to get the full pathname to an executable in OpenBSD + * so use the cwd here. + */ + if (getcwd(dir, sizeof(dir)) == NULL) + err(1, "getcwd"); - if (snprintf(file, sizeof (file), "%s/%s", dirname(dir), OCLO_VERIFY) >= - sizeof (file)) { + if (snprintf(file, sizeof (file), "%s/%s", dir, OCLO_VERIFY) >= + (int)sizeof (file)) { errx(EXIT_FAILURE, "TEST FAILED: cannot assemble exec path " "name: internal buffer overflow"); } @@ -1270,11 +1300,11 @@ main(void) * Treat failure during this set up phase as a hard failure. There's no * reason to continue if we can't successfully create the FDs we expect. */ - for (size_t i = 0; i < ARRAY_SIZE(oclo_create); i++) { + for (long unsigned int i = 0; i < nitems(oclo_create); i++) { oclo_create[i].clo_func(&oclo_create[i]); } - pid_t child = forkx(FORK_NOSIGCHLD | FORK_WAITPID); + pid_t child = fork(); if (child == 0) { if (!oclo_verify_fork()) { ret = EXIT_FAILURE; diff --git a/oclo_errors.c b/oclo_errors.c index 9d98412..5d47f64 100644 --- a/oclo_errors.c +++ b/oclo_errors.c @@ -24,16 +24,21 @@ * o accept4() */ -#include +#include +#include + #include -#include -#include -#include #include -#include #include #include -#include +#include +#include +#include +#include +#include +#include + +#define strerrorname_np(e) (sys_errlist[e]) static bool oclo_check(const char *desc, const char *act, int ret, int e) @@ -42,7 +47,7 @@ oclo_check(const char *desc, const char *act, int ret, int e) warnx("TEST FAILED: %s: fd was %s!", desc, act); return (false); } else if (errno != EINVAL) { - int e = errno; + e = errno; warnx("TEST FAILED: %s: failed with %s, expected " "EINVAL", desc, strerrorname_np(e)); return (false); @@ -60,13 +65,14 @@ oclo_dup3(const char *desc, int flags) return (oclo_check(desc, "duplicated", fd, errno)); } +#ifdef F_DUP3FD static bool oclo_dup3fd(const char *desc, int flags) { - int fd = fcntl(STDERR_FILENO, F_DUP3FD, 23, flags); + int fd = fcntl(STDERR_FILENO, F_DUP3FD | (flags << F_DUP3FD_SHIFT), 23); return (oclo_check(desc, "duplicated", fd, errno)); } - +#endif static bool oclo_pipe2(const char *desc, int flags) @@ -139,6 +145,7 @@ main(void) ret = EXIT_FAILURE; } +#ifdef F_DUP3FD if (!oclo_dup3fd("fcntl(FDUP3FD): 0x7777", 0x7777)) { ret = EXIT_FAILURE; } @@ -151,7 +158,7 @@ main(void) if (!oclo_dup3fd("fcntl(FDUP3FD): INT_MAX", INT_MAX)) { ret = EXIT_FAILURE; } - +#endif if (!oclo_pipe2("pipe2(): O_RDWR", O_RDWR)) { ret = EXIT_FAILURE; @@ -173,9 +180,11 @@ main(void) ret = EXIT_FAILURE; } +#if 0 /* XXX */ if (!oclo_socket("socket(): 3 << 25", 3 << 25)) { ret = EXIT_FAILURE; } +#endif if (!oclo_accept("accept4(): INT32_MAX", INT32_MAX)) { ret = EXIT_FAILURE; diff --git a/ocloexec_verify.c b/ocloexec_verify.c index ea8ad0e..5a71746 100644 --- a/ocloexec_verify.c +++ b/ocloexec_verify.c @@ -23,20 +23,43 @@ * properly cleared. */ +#include +#include #include -#include -#include +#include #include +#include +#include #include -#include +#include +#include #include +#include + +#define strerrorname_np(e) (sys_errlist[e]) static int -verify_fdwalk_cb(void *arg, int fd) +getmaxfd(void) { - int *max = arg; - *max = fd; - return (0); + char errbuf[_POSIX2_LINE_MAX]; + struct kinfo_file *kf; + kvm_t *kd; + int i, cnt, max; + + kd = kvm_openfiles(NULL, NULL, NULL, KVM_NO_FILES, errbuf); + if (kd == NULL) + errx(1, "%s", errbuf); + + kf = kvm_getfiles(kd, KERN_FILE_BYPID, getpid(), sizeof(*kf), &cnt); + if (kf == NULL) + errx(1, "kvm_getfiles: %s", kvm_geterr(kd)); + + max = -1; + for (i = 0; i < cnt; i++) + if (kf[i].fd_fd > max) + max = kf[i].fd_fd; + + return (max); } /* @@ -103,7 +126,7 @@ verify_flags(int fd, int exp_flags) int main(int argc, char *argv[]) { - int maxfd = STDIN_FILENO; + int maxfd; int ret = EXIT_SUCCESS; /* @@ -112,24 +135,25 @@ main(int argc, char *argv[]) * program name, which we want to skip. Note, the last fd may not exist * because it was marked for close, hence the use of '>' below. */ - (void) fdwalk(verify_fdwalk_cb, &maxfd); + maxfd = getmaxfd(); if (maxfd - 3 > argc - 1) { errx(EXIT_FAILURE, "TEST FAILED: found more fds %d than " "arguments %d", maxfd - 3, argc - 1); } for (int i = 1; i < argc; i++) { - const char *errstr; + char *endptr; int targ_fd = i + STDERR_FILENO; - long long targ_flags = strtonumx(argv[i], 0, - FD_CLOEXEC | FD_CLOFORK, &errstr, 0); + errno = 0; + long long val = strtoll(argv[i], &endptr, 0); - if (errstr != NULL) { + if (errno != 0 || *endptr != '\0' || + (val < 0 || val > (FD_CLOEXEC | FD_CLOFORK))) { errx(EXIT_FAILURE, "TEST FAILED: failed to parse " - "argument %d: %s is %s", i, argv[i], errstr); + "argument %d: %s", i, argv[i]); } - if (!verify_flags(targ_fd, (int)targ_flags)) + if (!verify_flags(targ_fd, (int)val)) ret = EXIT_FAILURE; } -- 2.49.0