[PATCH] D65221: [Sanitizer][ASAN][MSAN] Fix infinite recursion on FreeBSD

[Alexander Richardson via Phabricator via llvm-commits]

arichardson marked an inline comment as done.
arichardson added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux.cc:780
 #if SANITIZER_FREEBSD
+typedef int (*syctlbyname_ptr)(const char *sname, void *oldp, size_t *oldlenp,
+                          const void *newp, size_t newlen);
----------------
MaskRay wrote:
> MaskRay wrote:
> > Change it to `using` and place it inside the function.
> If the signature of `internal_sysctlbyname` is the same as `sysctlbyname`, I believe the below will be simpler:
> 
> ```
> static void *real;
> if (!real)
>   real = dlsym(RTLD_NEXT, "sysctlbyname");
> return (decltype(internal_sysctlbyname)*(real))(sname, oldp, oldlenp, newp, newlen);
> ```
> 
> I wonder if FreeBSD specific dlfunc is still relevant nowadays...
> 
> > FreeBSD dlsym(3): "in the C standard, conversions between data and function pointer types are undefined"
> >
> 
> Yet, C11 J.5.7 Function pointer casts (Annex J. I guess all working C compilers support this..):
> 
> > A pointer to an object or to void may be cast to a pointer to a function, allowing data to be invoked as a function (6.5.4).
> 
> C++11 [expr.reinterpret.cast]
> 
> > Converting a function pointer to an object pointer type or vice versa is conditionally-supported.
Theoretically, function pointers could be a different size from data pointers (e.g. a pair of pointers for code+got, or even something smaller if you have separate code and data address spaces). But since so much C code assumes that you can store them in a `void*` I'm not sure any relevant architecture actually uses different sizes.

I quite like `dlfunc()` because using it explicitly states that you are looking up a function and not a data symbol. FreeBSD rtld doesn't actually differntiate it right now, but I was considering changing it to return NULL if the lookup yields a non-function symbol.

However, using dlfunc() is only fine because this is in a FreeBSD specific block so if other operating systems want to use this function it would need to change. I don't mind changing it to use dlsym() if you think that may be the case in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65221/new/

https://reviews.llvm.org/D65221