[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);
----------------
krytarowski wrote:
> arichardson wrote:
> > krytarowski wrote:
> > > arichardson wrote:
> > > > arichardson wrote:
> > > > > 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.
> > > > Unfortunately uptr* is not the same as size_t*. I'm not sure why the signature is different but changing the signature would require updating the other implementations, too.
> > > size_t is popularly replaced with uptr that in practice is the same underlying type.
> > I believe I got an error without the cast. Maybe for i386 size_t is int and uptr long? Unrelated to this review, the system that I work on (CHERI on FreeBSD) has 64-bit size_t and 128-bit uinptr_t so they are definitely not always compatible.
> > 
> > Also these casts were already required before my patch.
> `128-bit uinptr_t`? 128bit pointers?
> 
> If there are 128 bit pointers then there are probably bigger problems than this cast here.
Yes exactly (see http://www.chericpu.com for more details). But as I said this is unrelated to this review.

This change is just about making ASAN work again on FreeBSD (x86).
It's been broken for all of 8.0 and it would be very nice to have it working again before 9.0 is shipped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65221