[rtsan][tsan] Fix va_args handling in open functions

[cjappl]

This PR is being opened in response to this comment

Check oflag to see if it contains O_CREAT before unpacking parameters

EDIT: please see comments below discussing whether or not this is necessary

[cjappl]

CC for review @davidtrevelyan

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Chris Apple (cjappl)

Changes

It is only valid to read from the variadic parameter if O_CREAT is specified in the input flags, otherwise we should pass nothing in.

---

Full diff: https://github.com/llvm/llvm-project/pull/108291.diff

1 Files Affected:

• (modified) compiler-rt/lib/rtsan/rtsan_interceptors.cpp (+16-12)

diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
index 409e27c3ad3234..a9398cf284d827 100644
--- a/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
@@ -64,13 +64,15 @@ INTERCEPTOR(int, open, const char *path, int oflag, ...) {
   // O_NONBLOCK
   __rtsan_expect_not_realtime("open");
 
-  va_list args;
-  va_start(args, oflag);
-  const mode_t mode = va_arg(args, int);
-  va_end(args);
+  if (oflag & O_CREAT) {
+    va_list args;
+    va_start(args, oflag);
+    const mode_t mode = va_arg(args, int);
+    va_end(args);
+    return REAL(open)(path, oflag, mode);
+  }
 
-  const int result = REAL(open)(path, oflag, mode);
-  return result;
+  return REAL(open)(path, oflag);
 }
 
 INTERCEPTOR(int, openat, int fd, const char *path, int oflag, ...) {
@@ -78,13 +80,15 @@ INTERCEPTOR(int, openat, int fd, const char *path, int oflag, ...) {
   // O_NONBLOCK
   __rtsan_expect_not_realtime("openat");
 
-  va_list args;
-  va_start(args, oflag);
-  mode_t mode = va_arg(args, int);
-  va_end(args);
+  if (oflag & O_CREAT) {
+    va_list args;
+    va_start(args, oflag);
+    const mode_t mode = va_arg(args, int);
+    va_end(args);
+    return REAL(openat)(fd, path, oflag, mode);
+  }
 
-  const int result = REAL(openat)(fd, path, oflag, mode);
-  return result;
+  return REAL(openat)(fd, path, oflag);
 }
 
 INTERCEPTOR(int, creat, const char *path, mode_t mode) {

[cjappl]

Hmm, as a counter-argument to this PR, it seems like tsan does the same thing that we do now, always unpacking the arg:

llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

Lines 1664 to 1675 in c2b93e0

	TSAN_INTERCEPTOR(int, open, const char *name, int oflag, ...) {
	va_list ap;
	va_start(ap, oflag);
	mode_t mode = va_arg(ap, int);
	va_end(ap);
	SCOPED_TSAN_INTERCEPTOR(open, name, oflag, mode);
	READ_STRING(thr, pc, name, 0);
	int fd = REAL(open)(name, oflag, mode);
	if (fd >= 0)
	FdFileCreate(thr, pc, fd);
	return fd;
	}

If this flag is not O_CREAT, the underlying implementation should never touch the third argument, avoiding UB, right?

This is less important for these two functions, and more important to fcntl where the handling of all the cases would be crazy.

[cjappl]

@kubamracek - could you weigh in on if this is necessary or not? I see you did the original work adding va_args to the tsan open interceptor.

Is there anything unsafe about the original way we were doing this?

[nikic]

LGTM

[vitalybuka]

I would expect it's unnecessary? it probably unpacks garbage, but it's not suppose to be used.

also seem O_TMPFILE can read mode?

[vitalybuka]

LGTM, but I'd suggest either not commit, or include TSAN as well.

[cjappl]

Thanks for the additional tests and support vitalybuka