Problem with realpath interceptor

[jakubjelinek]

Summary

tsan in some cases (e.g. after fork from multithreaded program, which arguably is problematic) increments ignore_interceptors and in that case runs just the intercepted functions and not their wrappers.
For realpath the interceptor handles the resolved_path == nullptr case though and so when ignore_interceptors is non-zero, realpath (".", nullptr) will fail instead of succeeding.
This patch uses instead the COMMON_INTERCEPT_FUNCTION_GLIBC_VER_MIN macro to use realpath@@GLIBC_2.3 whenever possible (if not, then it is likely a glibc architecture
with more recent oldest symbol version than 2.3, for which any realpath in glibc will DTRT, or unsupported glibc older than 2.3), which never supported NULL as second argument.

[dvyukov]

Hi Jakub,
I don't remember if you have commit access or not. Do you want me to land this?

[jakubjelinek]

In D107819#2936888, @dvyukov wrote:

I don't remember if you have commit access or not. Do you want me to land this?

I don't have access. So yes, please commit this.

[MaskRay]

git commit --amend --author='name <email>' can be used for proper attribution in the future.

[dvyukov]

In D107819#2938206, @MaskRay wrote:

git commit --amend --author='name <email>' can be used for proper attribution in the future.

I used arc. I assumed it will do the right thing. It even asked about landing a patch by a different author.
Also I don't even see the email provided anywhere...

[MaskRay]

In D107819#2938645, @dvyukov wrote:

In D107819#2938206, @MaskRay wrote:

git commit --amend --author='name <email>' can be used for proper attribution in the future.

I used arc. I assumed it will do the right thing. It even asked about landing a patch by a different author.

arc patch D107819 does the right thing if the Differential was uploaded via arc diff 'HEAD^'.
It seems it isn't, so the author name/email are not tracked.

Also I don't even see the email provided anywhere...

I assume that you know Jakub's email... otherwise you may ask for the information:)

[dvyukov]

In D107819#2938677, @MaskRay wrote:

In D107819#2938645, @dvyukov wrote:

In D107819#2938206, @MaskRay wrote:

git commit --amend --author='name <email>' can be used for proper attribution in the future.

I used arc. I assumed it will do the right thing. It even asked about landing a patch by a different author.

arc patch D107819 does the right thing if the Differential was uploaded via arc diff 'HEAD^'.
It seems it isn't, so the author name/email are not tracked.

Also I don't even see the email provided anywhere...

I assume that you know Jakub's email... otherwise you may ask for the information:)

Thanks, will use in future.

[vitalybuka]

I had to revert the patch because it breaks interceptor on some environments. I'll investigate the issue.

If TSAN issue is important to you, probably moving WRAP(malloc) workaround before COMMON_INTERCEPTOR_ENTER will fix it. I will accept the patch if are willing to try.
Could you also try to build some regression test for the TSAN issue?

[jakubjelinek]

What kind of problems are you talking about? Is it that some non-glibc system wants also the NULL resolved_path handling and doesn't have it in the libc?
To be precise, the patch would work fine if only the INIT_REALPATH macro is changed to the COMMON_INTERCEPT_FUNCTION_GLIBC_VER_MIN macro, but no reordering will help,
the problem is that tsan disables the wrappers altogether at some points and then it only calls the original function. And it is crucial that the original function is the right one.

[vitalybuka]

In D107819#2966706, @jakubjelinek wrote:

What kind of problems are you talking about? Is it that some non-glibc system wants also the NULL resolved_path handling and doesn't have it in the libc?

It's GLIBC, it has all functions, but COMMON_INTERCEPT_FUNCTION_GLIBC_VER_MIN fallbacks to the old one. Patch works without fallback, but it will break non-glibc cases.
E.g. it will work COMMON_INTERCEPT_FUNCTION_VER which does not fallback

[vitalybuka]

Also fallback it's quite recent thing D96348

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

enum {
     CIMT_FILE
   } type;
   union {
     FileMetadata file;
   };
 };
 
 #if SI_POSIX
 typedef AddrHashMap<CommonInterceptorMetadata, 31051> MetadataHashMap;
 
 static MetadataHashMap *interceptor_metadata_map;
 
 UNUSED static void SetInterceptorMetadata(__sanitizer_FILE *addr,
                                           const FileMetadata &file) {
   MetadataHashMap::Handle h(interceptor_metadata_map, (uptr)addr);
   CHECK(h.created());
   h->type = CommonInterceptorMetadata::CIMT_FILE;
   h->file = file;
 }
 
 UNUSED static const FileMetadata *GetInterceptorMetadata(
     __sanitizer_FILE *addr) {

 
 UNUSED static void DeleteInterceptorMetadata(void *addr) {
   MetadataHashMap::Handle h(interceptor_metadata_map, (uptr)addr, true);
   CHECK(h.exists());
 }
 #endif  // SI_POSIX
 
 #if SANITIZER_INTERCEPT_STRLEN
 INTERCEPTOR(SIZE_T, strlen, const char *s) {
   // Sometimes strlen is called prior to InitializeCommonInterceptors,
   // in which case the REAL(strlen) typically used in
   // COMMON_INTERCEPTOR_ENTER will fail.  We use internal_strlen here
   // to handle that.
   if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
     return internal_strlen(s);
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strlen, s);
   SIZE_T result = REAL(strlen)(s);
   if (common_flags()->intercept_strlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, s, result + 1);
   return result;
 }
 #define INIT_STRLEN COMMON_INTERCEPT_FUNCTION(strlen)
 #else
 #define INIT_STRLEN
 #endif
 
 #if SANITIZER_INTERCEPT_STRNLEN
 INTERCEPTOR(SIZE_T, strnlen, const char *s, SIZE_T maxlen) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strnlen, s, maxlen);
   SIZE_T length = REAL(strnlen)(s, maxlen);
   if (common_flags()->intercept_strlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, s, Min(length + 1, maxlen));
   return length;
 }
 #define INIT_STRNLEN COMMON_INTERCEPT_FUNCTION(strnlen)
 #else
 #define INIT_STRNLEN
 #endif
 
 #if SANITIZER_INTERCEPT_STRNDUP
 INTERCEPTOR(char*, strndup, const char *s, uptr size) {
   void *ctx;
   COMMON_INTERCEPTOR_STRNDUP_IMPL(ctx, s, size);
 }
 #define INIT_STRNDUP COMMON_INTERCEPT_FUNCTION(strndup)
 #else
 #define INIT_STRNDUP
 #endif // SANITIZER_INTERCEPT_STRNDUP
 

 #define INIT_TCGETATTR
 #endif
 
 #if SANITIZER_INTERCEPT_REALPATH
 INTERCEPTOR(char *, realpath, const char *path, char *resolved_path) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, realpath, path, resolved_path);
   if (path) COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path) + 1);
-
-  // Workaround a bug in glibc where dlsym(RTLD_NEXT, ...) returns the oldest
-  // version of a versioned symbol. For realpath(), this gives us something
-  // (called __old_realpath) that does not handle NULL in the second argument.
-  // Handle it as part of the interceptor.
-  char *allocated_path = nullptr;
-  if (!resolved_path)
-    allocated_path = resolved_path = (char *)WRAP(malloc)(path_max + 1);
-
   char *res = REAL(realpath)(path, resolved_path);
-  if (allocated_path && !res) WRAP(free)(allocated_path);
   if (res) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, res, REAL(strlen)(res) + 1);
   return res;
 }
-#define INIT_REALPATH COMMON_INTERCEPT_FUNCTION(realpath);
+#define INIT_REALPATH COMMON_INTERCEPT_FUNCTION_GLIBC_VER_MIN(realpath, "GLIBC_2.3");
 #else
 #define INIT_REALPATH
 #endif
 
 #if SANITIZER_INTERCEPT_CANONICALIZE_FILE_NAME
 INTERCEPTOR(char *, canonicalize_file_name, const char *path) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, canonicalize_file_name, path);