Introduce vfcntl() #2194
Introduce vfcntl() #2194
Conversation
Add a function that takes a va_list argument to avoid duplication the switch statement. While touching this, also use __libsys_interposing_slot() directly instead of indirecting through __libc_interposing_slot().
This is a variant of fcntl that takes a va_list instead of variadic arguments and can be used to safely interpose fcntl by libraries such as epoll-shim that would otherwise have to reimplement the logic inside fcntl.c to avoid reading incorrect arguments for CheriBSD. See jiixyj/epoll-shim#36
| @@ -383,7 +383,8 @@ FBSD_1.7 { | |||
| }; | |||
|
|
|||
| FBSD_1.8 { | |||
| kcmp; | |||
There was a problem hiding this comment.
I was going to upstream this whitespace cleanup but turns out that file does not exist in FreeBSD.
…ents `fcntl()` abuses C variadic arguments to implement optional parameters. We want to forward this argument to the real `fcntl()` call, but doing so is non-portable. On most architectures, we can just read an `intptr_t`/`void*` and this will give us a (semi-)valid result even if no argument/an int was passed instead since `va_arg()` will generally read the next argument register or arbitrary data from the stack. On CHERI-enabled architectures, variadic arguments are tightly bounded, which means that reading a `void*` if an `int` was passed will result in a runtime trap. To avoid this problem this commit uses a new `vfcntl` libc function (CTSRD-CHERI/cheribsd#2194), that takes a va_list instead of variadic arguments. Since this needs a new HAVE_VCNTL compile-time check, this adds a new config.h header that also includes the HAVE_TIMERFD value. This removes the need to pass -D flags on the command line. This reduces the number of test failures on CheriBSD Morello from 210 to 8.
There was a problem hiding this comment.
I like vfcntl(), but it definitely belongs in libc as does the main fcntl(). I think I wrongly didn't follow upstream in moving fcntl.c because _fcntl() would ordinarily end up in libsys (I need to revisit that). I think fcntl.c should be moved to libc with a "Fixes: d7847a8" note.
Yeah I was a bit confused about the mismatch between upstream and our local libsys. Splitting those files into fcntl and _fcntl would bring back the duplication unless we include the same file and just use a pre-defined macro? |
I think I'd just move _fcntl to libc for now. You can ignore the _fcntl symbol in lib/libsys/syscalls.map for now and I'll clean it up later. I've got a few unfinished TODOs with the _ symbols, but am waiting for freebsd/freebsd-src#1362 to touch the syscall generation code more. |
| @@ -80,16 +82,24 @@ call_fcntl(fcntl_func fn, int fd, int cmd, va_list ap) | |||
| return (fn(fd, cmd, arg)); | |||
| } | |||
|
|
|||
| #pragma weak fcntl | |||
|
Do we really want to be adding this as a downstream CheriBSD thing? |
That's a fair point, I'd be happy to try and upstream this? I'm not sure how many other libraries want to interpose fcntl but it would be good to give them a non-UB way to do so. |
|
I think it would be nice to add vfcntl() upstream to FreeBSD. |
|
Submitted upstream as https://reviews.freebsd.org/D46403 - will change this to a cherry-pick + merge conflict resolution if accepted. |
This is a variant of fcntl that takes a va_list instead of variadic
arguments and can be used to safely interpose fcntl by libraries such
as epoll-shim that would otherwise have to reimplement the logic inside
fcntl.c to avoid reading incorrect arguments for CheriBSD.
See jiixyj/epoll-shim#36