[SanitizerCommon] add null check for fopen64 interceptor

[yingcong-wu]

Currently, the interceptor for fopen64 will crash when path is null. Adding the same null check as fopen().

[yingcong-wu]

Hi @kcc , I don't have access to add reviewers, could you please review this patch? Thanks.

[llvmbot]

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

Author: Wu Yingcong (yingcong-wu)

Changes

Currently, the interceptor for fopen64 will crash when path is null. Adding the same null check as fopen().

---

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

2 Files Affected:

• (modified) compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc (+1-1)
• (added) compiler-rt/test/sanitizer_common/TestCases/fopen64_nullptr.c (+9)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
index 80efaf54a0607f6..4da29d928fcc236 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
@@ -6145,7 +6145,7 @@ INTERCEPTOR(int, flopenat, int dirfd, const char *path, int flags, ...) {
 INTERCEPTOR(__sanitizer_FILE *, fopen64, const char *path, const char *mode) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, fopen64, path, mode);
-  COMMON_INTERCEPTOR_READ_RANGE(ctx, path, internal_strlen(path) + 1);
+  if (path) COMMON_INTERCEPTOR_READ_RANGE(ctx, path, internal_strlen(path) + 1);
   COMMON_INTERCEPTOR_READ_RANGE(ctx, mode, internal_strlen(mode) + 1);
   __sanitizer_FILE *res = REAL(fopen64)(path, mode);
   COMMON_INTERCEPTOR_FILE_OPEN(ctx, res, path);
diff --git a/compiler-rt/test/sanitizer_common/TestCases/fopen64_nullptr.c b/compiler-rt/test/sanitizer_common/TestCases/fopen64_nullptr.c
new file mode 100644
index 000000000000000..2c260865c80a792
--- /dev/null
+++ b/compiler-rt/test/sanitizer_common/TestCases/fopen64_nullptr.c
@@ -0,0 +1,9 @@
+// Check that fopen64(NULL, "r") is ok.
+// `-m32` and `-D_FILE_OFFSET_BITS=64` will make fopen() call fopen64()
+
+// REQUIRES: asan
+// RUN: %clang -m32 -D_FILE_OFFSET_BITS=64 -O2 %s -o %t && %run %t
+#include <stdio.h>
+const char *fn = NULL;
+FILE *f;
+int main() { f = fopen(fn, "r"); }

[yingcong-wu]

My test is not well-written. I don't know how fopen64() is used in other systems, so I don't know if I can test fopen64() directly.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[thurstond]

The interceptors file defines

#define SANITIZER_INTERCEPT_FOPEN64 (SI_GLIBC || SI_SOLARIS32)

so // REQUIRES: glibc might be a safer bet? (and technically solaris32, but no one will notice). Not all Linux builds will have glibc.

[thurstond]

After adding back the 'RUN' command but not making the interceptor change:

==2130402==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x566e2bb0 bp 0xff8ee478 sp 0xff8ee02c T0)
==2130402==The signal is caused by a READ memory access.
==2130402==Hint: address points to the zero page.
    #0 0x566e2bb0 in __sanitizer::internal_strlen(char const*) /usr/local/google/home/thurston/llvm-projectC/compiler-rt/lib/sanitizer_common/sanitizer_libc.cpp:176:10
    #1 0x566894cc in fopen64 /usr/local/google/home/thurston/llvm-projectC/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:6148:3
    #2 0x5670d2c4 in main /usr/local/google/home/thurston/llvm-projectC/compiler-rt/test/sanitizer_common/TestCases/fopen64_nullptr.c:8:18
    #3 0xf7c237c4  (/lib/i386-linux-gnu/libc.so.6+0x237c4) (BuildId: a098c9624e774a2b27e7cc84a9bb6290f93d26c4)
    #4 0xf7c23887 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x23887) (BuildId: a098c9624e774a2b27e7cc84a9bb6290f93d26c4)
    #5 0x56623346 in _start (/usr/local/google/home/thurston/llvm-projectC/build/projects/compiler-rt/test/sanitizer_common/asan-i386-Linux/Output/fopen64_nullptr.c.tmp+0x19346)

This is good: your test resulted in a failure when the interceptor code is wrong

After also making the interceptor change:

clang: error: unsupported option '-fsanitize=hwaddress' for target 'i386-unknown-linux-gnu'
...
clang: error: unsupported option '-fsanitize=thread' for target 'i386-unknown-linux-gnu'
...
/usr/bin/ld: cannot find /usr/local/google/home/thurston/llvm-projectC/build/lib/clang/18/lib/linux/libclang_rt.msan-i386.a: No such file or directory
...
Failed Tests (3):
  SanitizerCommon-hwasan-x86_64-Linux :: fopen64_nullptr.c
  SanitizerCommon-msan-x86_64-Linux :: fopen64_nullptr.c
  SanitizerCommon-tsan-x86_64-Linux :: fopen64_nullptr.c


Testing Time: 0.33s
  Excluded: 1989
  Passed  :    6
  Failed  :    3

so I think it also needs something like:

// UNSUPPORTED: hwasan, msan, tsan

or you could try changing the compilation options to remove the 32-bit build (haven't tried it myself).

[MaskRay]

https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html says "The fopen() function shall open the file whose pathname is the string pointed to by pathname, and associates a stream with it." A null argument cannot be used and the behavior is undefined. I cannot find historical documented precedent for accepting a null argument in https://www.kernel.org/doc/man-pages/ and elsewhere. There are many places that we don't check null arguments for COMMON_INTERCEPTOR_WRITE_RANGE and the like when passing null arguments causes UB. I think this patch should be rejected.

[MaskRay]

.

[thurstond]

https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html says "The fopen() function shall open the file whose pathname is the string pointed to by pathname, and associates a stream with it." A null argument cannot be used and the behavior is undefined. I cannot find historical documented precedent for accepting a null argument in https://www.kernel.org/doc/man-pages/ and elsewhere. There are many places that we don't check null arguments for COMMON_INTERCEPTOR_WRITE_RANGE and the like when passing null arguments causes UB. I think this patch should be rejected.

Since it's UB, we can do whatever we like, so accepting the null argument is not technically wrong, right?

Alternatively, would you be satisfied if the interceptor were changed to return an error in such cases?

I don't like the status quo (i.e., without this patch), whereby the interceptor will dereference a null pointer (e.g., output in #68760 (comment)), because it's not an intuitive error message.

[yingcong-wu]

Thank you @thurstond for your help. I added back the RUN line and add requires for asan and glibc.

I think we should add this since we already have added a null check for fopen() in https://github.com/intel-restricted/applications.compilers.llvm-project/commit/1d1be3dd8822c18babbebdcc60d4bbf152df5607.

[yingcong-wu]

Closed as reviewer rejected.