[asan] Fix the freopen interceptor to allow NULL instead of a filename

[kubamracek]

Summary

According to man freopen, passing NULL instead of a filename is valid, however the current implementation of the interceptor assumes this parameter is non-NULL. Let's fix that and add a test case.

[glider]

The case of path==NULL is not covered in the Ubuntu manpage for freopen, but this behavior is mandated by POSIX and is in fact implemented in Glibc.
Therefore LGTM

test/asan/TestCases/Posix/freopen.cc

11

Please add the return statement.

[kubamracek]

Hm, the Linux buildbot didn't like this change: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/19256/steps/check-asan%20in%20gcc%20build/logs/stdio

Interestingly, only the i386 variant failed. Could it be that that system's libc really doesn't support NULL for freopen? Alexander, could you help me out here? I reverted the patch until we figure this out.

Thanks!

[glider]

When built with -m32 and without ASan, this testcase does not crash for me.
Trying to reproduce with ASan.

[kubamracek]

In D11389#209038, @glider wrote:

When built with -m32 and without ASan, this testcase does not crash for me.
Trying to reproduce with ASan.

Hi Alexander, did you find any cause of the test failure? Thanks!

[glider]

Hi Kuba,

the crash reproduced for me, but I couldn't find the cause, and then I
got distracted.
I'll try to take a look again next week.

Alex

[kubamracek]

OK, I was able to reproduce the i386 Linux test failure. It looks like it has little to do with the actual patch/fix, but instead it's a more general issue with versioned symbols from glibc – freopen is a versioned symbol (and incompatible between versions), and when it's intercepted, a wrong version is called. Even this fails (with or without this patch):

FILE *fp = fopen("/dev/null", "w");
freopen("/dev/null", "r", fp);
fclose(fp);

This seems to be a Linux-specific issue, and frankly, I don't know what to do about it. In the meantime, I'm suggesting to commit this patch without the testcase. Updating the diff.

[glider]

I suggest to commit the patch anyway, but mark it as disabled.

[kubamracek]

In D11389#234173, @glider wrote:

I suggest to commit the patch anyway, but mark it as disabled.

What's the proper way to mark a test as disabled? I'm only aware of XFAIL and REQUIRES marks.

[glider]

I think XFAIL is fine.

[kubamracek]

In D11389#234175, @glider wrote:

I think XFAIL is fine.

I don't think that'll work, since this only fails on i386 Linux and works fine on other platforms and architectures...

[kubamracek]

Marking the patch with REQUIRES: asan-64-bits to skip the test on i386 Linux (where it fails due to an unrelated issue with glibc versioned symbols). XFAIL wouldn't work here, because the test actually passes on all other platforms/architectures.

[kubamracek]

So is this okay to commit?

[glider]

Yes. Please go ahead.

[kubamracek]

Landed in r246435.

lib/sanitizer_common/sanitizer_common_interceptors.inc

INTERCEPTOR(__sanitizer_FILE *, fdopen, int fd, const char *mode) {
   __sanitizer_FILE *res = REAL(fdopen)(fd, mode);
   if (res) unpoison_file(res);
   return res;
 }
 INTERCEPTOR(__sanitizer_FILE *, freopen, const char *path, const char *mode,
             __sanitizer_FILE *fp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, freopen, path, mode, fp);
-  COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path) + 1);
+  if (path) COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path) + 1);
   COMMON_INTERCEPTOR_READ_RANGE(ctx, mode, REAL(strlen)(mode) + 1);
   COMMON_INTERCEPTOR_FILE_CLOSE(ctx, fp);
   __sanitizer_FILE *res = REAL(freopen)(path, mode, fp);
   COMMON_INTERCEPTOR_FILE_OPEN(ctx, res, path);
   if (res) unpoison_file(res);
   return res;
 }
 #define INIT_FOPEN                   \
INTERCEPTOR(__sanitizer_FILE *, fopen64, const char *path, const char *mode) {
   COMMON_INTERCEPTOR_FILE_OPEN(ctx, res, path);
   if (res) unpoison_file(res);
   return res;
 }
 INTERCEPTOR(__sanitizer_FILE *, freopen64, const char *path, const char *mode,
             __sanitizer_FILE *fp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, freopen64, path, mode, fp);
-  COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path) + 1);
+  if (path) COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path) + 1);
   COMMON_INTERCEPTOR_READ_RANGE(ctx, mode, REAL(strlen)(mode) + 1);
   COMMON_INTERCEPTOR_FILE_CLOSE(ctx, fp);
   __sanitizer_FILE *res = REAL(freopen64)(path, mode, fp);
   COMMON_INTERCEPTOR_FILE_OPEN(ctx, res, path);
   if (res) unpoison_file(res);
   return res;
 }
 #define INIT_FOPEN64                  \

test/asan/TestCases/Posix/freopen.cc

+// RUN: %clangxx_asan -O0 %s -o %t && %run %t
+
+// This fails on i386 Linux due to a glibc versioned symbols mixup.
+// REQUIRES: asan-64-bits
+
+#include <assert.h>
+#include <stdio.h>
+
+int main() {
+  FILE *fp = fopen("/dev/null", "w");
+  assert(fp);
+  freopen(NULL, "a", fp);
+  fclose(fp);
+  return 0;
+}