[sanitizer] Don't call dlerror() after swift_demangle lookup through dlsym

[serge-sans-paille]

Summary

Because the call to dlerror() may actually want to print something, which turns into a deadlock
as showcased in #49223.

Instead rely on further call to dlsym to clear dlerror internal state if they
need to check the return status.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

61–63

Unrelated to the patch, but this is used only in sanitizer_symbolizer_mac.cpp, so it's better to be moved there.

76

Maybe better just make
SANITIZER_WEAK_ATTRIBUTE DemangleSwift()
and remove swift_demangle_ft related stuff?

[serge-sans-paille]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

76

The idea of this patch is to delegate detection of the swift_demangle symbol from dlsym to weak/strong symbol. I don't see how making DemangleSwift weak achieves that goal?

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

61–63

My mistake, it's not just mac

66

shouldn't this be like:

extern "C" SANITIZER_WEAK_ATTRIBUTE swift_demangle(
   const char *mangledName,
   size_t mangledNameLength, char *outputBuffer,
   size_t *outputBufferSize, uint32_t flags);

78

why do we need InitializeSwiftDemangler here at all
can we just check if (swift_demangle) in DemangleSwift?

[serge-sans-paille]

Follow reviewer's advice

[serge-sans-paille]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

66

I don't think so, because in that case if the symbol is not available externally we get a link error.

78

Correct!

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

66

I believe it should be

// As of now, there are no headers for the Swift runtime. Provide a weak version
// of that symbol and rely on weak symbol resolution to pick the right one.
extern "C" char *SANITIZER_WEAK_ATTRIBUTE
swift_demangle(const char *mangledName, size_t mangledNameLength,
               char *outputBuffer, size_t *outputBufferSize, uint32_t flags);

// Attempts to demangle a Swift name. The demangler will return nullptr if a
// non-Swift name is passed in.
const char *DemangleSwift(const char *name) {
  if (&swift_demangle)
    return swift_demangle(name, internal_strlen(name), 0, 0, 0);
  return nullptr;
}

SANITIZER_WEAK_ATTRIBUTE makes it null if it's not linked, not linking error

"swift_demangle" symbol is a function, not function ptr, not sure how patch as is can work at all
Have you tried to run this with swift_demangle linked into the binary?

Maybe we should have a test with a fake version of the function
https://github.com/apple/swift/blob/de16c9a7385e61b6bf6dd604e832c64a02358d84/stdlib/public/runtime/Demangle.cpp#L721

505

does not compile with this line

[yln]

For the technicalities, I agree with Vitaly.

However, @rsundahl has discovered that the whole "strong overrides weak symbol across dylibs" pattern isn't reliably anymore. See here: https://reviews.llvm.org/D127929
Roy, can you confirm that we are running into this case here>

Also: does this still work right now? Should depend on the swift runtime dylib defining a weak symbol/being marked as in participating in weak symbol lookup. Can we promise that the Swift runtime dylib will always define a weak symbol?

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

66

I agree that this is what we would want if we take this path.

[rsundahl]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

505

What is the compiler complaining about in this case @vitalybuka?

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

505

it's trivial. @serge-sans-paille removed InitializeSwiftDemangler as I asked, but forgot to remove the call.

[rsundahl]

There are very significant differences between Linux, Windows and Darwin w.r.t. how weak symbols are (possibly) overridden by so-called "strong" symbols. The discussion with @MaskRay in https://reviews.llvm.org/D127929 is worth a look for context but I would encourage moving in the other direction, completely away from weak symbols and/or any expectation that the loader will "do what we want it to do".

[MaskRay]

In D128992#3643688, @rsundahl wrote:

There are very significant differences between Linux, Windows and Darwin w.r.t. how weak symbols are (possibly) overridden by so-called "strong" symbols. The discussion with @MaskRay in https://reviews.llvm.org/D127929 is worth a look for context but I would encourage moving in the other direction, completely away from weak symbols and/or any expectation that the loader will "do what we want it to do".

The file handles ELF systems and macOS. It doesn't deal with Windows.
Again, this is a use case which unnecessarily regressed on macOS with the latest dyld :(

[serge-sans-paille]

Another approach: make sure swift_demangle is setup before or after symbol lookup, but not during interception.
It solves the issue because it prevents calling the intiialization routine as a consequence of lazy initialization when hooking the malloc call from dlerror

[vitalybuka]

Please update description, as this approach does not match, it still uses dlsym.

Can you show a pseudo-stack trace of the case you are trying to solve?

[vitalybuka]

title is still "Don't lookup swift_demangle through dlsym"

Because it can fail if the symbol is not there, the subsequent (and legit) call to dlerror()

legit but unnecessary. Most code in sanitizer does not call dlerror after dlsym.
Doc says: https://linux.die.net/man/3/dlerror

the correct way to test for an error is to call dlerror() to clear any old error conditions, then call dlsym(), and then call dlerror() again, saving its return value into a variable, and check whether this saved value is not NULL.

We don't check dlerror here, so it's up to the next caller to clear it before next dlsym, and only if they care about dlerror.

[vitalybuka]

compiler-rt/lib/interception/interception_linux.cpp is the main and very early origin of dlsym calls for sanitizers

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

75

I like that we settled on this very simple (and reasonable) change to solve the problem.

LGTM, but deferring to Vitaly since he suggested the change! :)

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp

const char *DemangleCXXABI(const char *name) {
   // the returned value.
   if (&__cxxabiv1::__cxa_demangle)
     if (const char *demangled_name =
           __cxxabiv1::__cxa_demangle(name, 0, 0, 0))
       return demangled_name;
 
   return name;
 }
 
 // As of now, there are no headers for the Swift runtime. Once they are
 // present, we will weakly link since we do not require Swift runtime to be
 // linked.
 typedef char *(*swift_demangle_ft)(const char *mangledName,
                                    size_t mangledNameLength, char *outputBuffer,
                                    size_t *outputBufferSize, uint32_t flags);
 static swift_demangle_ft swift_demangle_f;
 
 // This must not happen lazily at symbolication time, because dlsym uses
 // malloc and thread-local storage, which is not a good thing to do during
 // symbolication.
 static void InitializeSwiftDemangler() {
   swift_demangle_f = (swift_demangle_ft)dlsym(RTLD_DEFAULT, "swift_demangle");
-  (void)dlerror(); // Cleanup error message in case of failure
 }
 
 // Attempts to demangle a Swift name. The demangler will return nullptr if a
 // non-Swift name is passed in.
 const char *DemangleSwift(const char *name) {
   if (swift_demangle_f)
     return swift_demangle_f(name, internal_strlen(name), 0, 0, 0);
 
   return nullptr;
 }
 
 const char *DemangleSwiftAndCXX(const char *name) {
Symbolizer *Symbolizer::PlatformInit() {
   IntrusiveList<SymbolizerTool> list;
   list.clear();
   ChooseSymbolizerTools(&list, &symbolizer_allocator_);
   return new(symbolizer_allocator_) Symbolizer(list);
 }
 
 void Symbolizer::LateInitialize() {
   Symbolizer::GetOrInit();
   InitializeSwiftDemangler();
 }
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_POSIX