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

[Kamil Rytarowski via Phabricator via llvm-commits]

krytarowski 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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65221