Skip to content

Commit 68f529d

Browse files
authored
[rtsan] Fix comment in fcntl, fix va_args type (#108440)
Two changes here: 1. Add significantly more detail on why this is OK, from the conversation here: https://discourse.llvm.org/t/how-to-write-an-interceptor-for-fcntl/81203 2. Change the type we expect from va_args to intptr_t, which was also a suggestion in that thread.
1 parent ea568a9 commit 68f529d

File tree

1 file changed

+26
-10
lines changed

1 file changed

+26
-10
lines changed

compiler-rt/lib/rtsan/rtsan_interceptors.cpp

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -157,22 +157,38 @@ INTERCEPTOR(int, creat64, const char *path, mode_t mode) {
157157
INTERCEPTOR(int, fcntl, int filedes, int cmd, ...) {
158158
__rtsan_notify_intercepted_call("fcntl");
159159

160-
va_list args;
161-
va_start(args, cmd);
162-
163160
// Following precedent here. The linux source (fcntl.c, do_fcntl) accepts the
164161
// final argument in a variable that will hold the largest of the possible
165-
// argument types (pointers and ints are typical in fcntl) It is then assumed
166-
// that the implementation of fcntl will cast it properly depending on cmd.
162+
// argument types. It is then assumed that the implementation of fcntl will
163+
// cast it properly depending on cmd.
167164
//
168-
// This is also similar to what is done in
169-
// sanitizer_common/sanitizer_common_syscalls.inc
170-
const unsigned long arg = va_arg(args, unsigned long);
171-
int result = REAL(fcntl)(filedes, cmd, arg);
165+
// The two types we expect for possible args are `struct flock*` and `int`
166+
// we will cast to `intptr_t` which should hold both comfortably.
167+
// Why `intptr_t`? It should fit both types, and it follows the freeBSD
168+
// approach linked below.
169+
using arg_type = intptr_t;
170+
static_assert(sizeof(arg_type) >= sizeof(struct flock *));
171+
static_assert(sizeof(arg_type) >= sizeof(int));
172+
173+
// Some cmds will not actually have an argument passed in this va_list.
174+
// Calling va_arg when no arg exists is UB, however all currently
175+
// supported architectures will give us a result in all three cases
176+
// (no arg/int arg/struct flock* arg)
177+
// va_arg() will generally read the next argument register or the
178+
// stack. If we ever support an arch like CHERI with bounds checking, we
179+
// may have to re-evaluate this approach.
180+
//
181+
// More discussion, and other examples following this approach
182+
// https://discourse.llvm.org/t/how-to-write-an-interceptor-for-fcntl/81203
183+
// https://reviews.freebsd.org/D46403
184+
// https://github.com/bminor/glibc/blob/c444cc1d8335243c5c4e636d6a26c472df85522c/sysdeps/unix/sysv/linux/fcntl64.c#L37-L46
172185

186+
va_list args;
187+
va_start(args, cmd);
188+
const arg_type arg = va_arg(args, arg_type);
173189
va_end(args);
174190

175-
return result;
191+
return REAL(fcntl)(filedes, cmd, arg);
176192
}
177193

178194
#if SANITIZER_INTERCEPT_FCNTL64

0 commit comments

Comments
 (0)