RTLD_DEEPBIND breaks sanitizer builds

[Bromeon]

Godot version

4.2-dev (7f4e700)

System information

Linux CI

Issue description

Change #82973 introduces a regression for a downstream memory-sanitizer job.

Here is an example of such a failed job, the error is:

==2646==You are trying to dlopen a /home/runner/.../debug/libitest.so shared library 
with RTLD_DEEPBIND flag which is incompatible with sanitizer runtime 
(see https://github.com/google/sanitizers/issues/611 for details).

If you want to run /home/runner/work/gdext/gdext/itest/godot/../../target/debug/libitest.so library 
under sanitizers please remove RTLD_DEEPBIND from dlopen flags.

While Godot CI runs sanitizer builds as well, I don't think they include GDExtension.

---

It would be nice if scons could be configured to not pass RTLD_DEEPBIND to dlopen().
Or maybe even better, automatically disable that behavior if any of the sanitizer flags is passed to scons.

Since the code has this check, it might even be enough to pass RTLD_DEEPBIND=0 as a global #define?

godot/drivers/unix/os_unix.cpp

Lines 84 to 86 in b137180

	#ifndef RTLD_DEEPBIND
	#define RTLD_DEEPBIND 0
	#endif

---

Steps to reproduce

Build Godot master with:

scons platform=linuxbsd target=editor dev_build=yes use_asan=yes use_ubsan=yes use_lsan=yes use_llvm=yes linker=lld

Load it with ASan/LSan environment variables, in a project that has a GDExtension.

Minimal reproduction project

N/A

[akien-mga]

I don't think this should be a compile option, I think this is pointed at the fact that RTLD_DEEPBIND may not be a safe option to use for all dynamic library loading.

This should maybe be an option in the GDExtension library's config, so users can opt-in to this behavior, and we keep the safe default.

[Bromeon]

This should maybe be an option in the GDExtension library's config, so users can opt-in to this behavior, and we keep the safe default.

Good idea! That would also allow different extensions with different settings.

---

Incidental to this, regarding sanitizer builds: in order to retain stacktraces in Rust code, it's necessary to not unload the dynamic library. A longer explanation can be seen in godot-rust/gdext#133.

I worked around this by applying a patch that removes these 3 lines:

godot/drivers/unix/os_unix.cpp

Lines 664 to 666 in b137180

	if (dlclose(p_library_handle)) {
	return FAILED;
	}

It looks like this was encountered in the past by Godot users as well: #46140 (comment)

It's relatively specific, but it might be something to think about if sanitizer builds + GDExtension are an officially supported setup.

[dsnopek]

I don't think this should be a compile option, I think this is pointed at the fact that RTLD_DEEPBIND may not be a safe option to use for all dynamic library loading.

Sanitizer builds are a pretty specific use case, and you already have to compile Godot with a special compile option to enable them, so I think making this a compile-time decision makes sense. Although, I'd argue it shouldn't be a new option, I think if you use any of the san options when building Godot, it should automatically disable RTLD_DEEPBIND.

This should maybe be an option in the GDExtension library's config, so users can opt-in to this behavior, and we keep the safe default.

I don't agree with this.

What happens without RTLD_DEEPBIND is super weird and unexpected (and Linux-specific), and I don't think any developer is going to know that they need to enable this.

To be clear, what happens without this option, is that Linux's dynamic linker will link any symbol in the GDExtension (defined or undefined) to a symbol in the main Godot executable with the same name. So, if you make a GDExtension that has a class called Object, even though your version of Object is compiled into the GDExtension and was fully defined at compile/link time, the dynamic linker will re-link all references to the Object in the Godot executable. (godot-cpp can have its Object class without problems because it's in a namespace, so its symbol name is actually godot::Object.)

Nobody ever wants this behavior :-)

I think it's only needed in sanitizer builds, because those builds are probably replacing malloc() or other system-level functions, and when you dynamically load a library, it needs that library to also use its replacement malloc(), etc.

So, again, I think it's sanitizer builds that are special here.

[akien-mga]

So what should we do here? Special case it for builds with sanitizers enabled? We have the SANITIZERS_ENABLED define that could be used for that.

[dsnopek]

So what should we do here? Special case it for builds with sanitizers enabled?

That was the original idea that @Bromeon and I discussed.

I have it on my TODO to try a different potential solution to the original problem that doesn't involve RTLD_DEEPBIND, but I'm not sure when I'll get around to that (or if it'll even work).

[dsnopek]

I just posted PR #84210 which should disable RTLD_DEEPBIND in sanitizer builds