Sanitizer built against glibc 2.34 doesn't work

[jakubjelinek]

Summary

As mentioned in https://gcc.gnu.org/PR100114 , glibc starting with the
https://sourceware.org/git/?p=glibc.git;a=commit;h=6c57d320484988e87e446e2e60ce42816bf51d53
change doesn't define SIGSTKSZ and MINSIGSTKSZ macros to constants, but to sysconf function call.
sanitizer_posix_libcdep.cpp has
static const uptr kAltStackSize = SIGSTKSZ * 4; // SIGSTKSZ is not enough.
which is generally fine, just means that when SIGSTKSZ is not a compile time constant will be initialized later.
The problem is that kAltStackSize is used in SetAlternateSignalStack which is called very early, from .preinit_array
initialization, i.e. far before file scope variables are constructed, which means it is not initialized and
mmapping 0 will fail:

145==ERROR: AddressSanitizer failed to allocate 0x0 (0) bytes of SetAlternateSignalStack (error code: 22)

Here is one possible fix, another one could be to make kAltStackSize a preprocessor macro if _SG_SIGSTKSZ is defined
(but perhaps with having an automatic const variable initialized to it so that sysconf isn't at least called twice
during SetAlternateSignalStack.

[marxin]

I do support the suggested change.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

166–172

Please remove preprocessor and replace with function and let compiler to optimize of it's a constant

[vitalybuka]

I guess you are not committer so I'll update and land it.

[vitalybuka]

update

[vitalybuka]

undo unrelated change

[jakubjelinek]

LGTM. For older glibc it will be most likely inlined and optimized into const like it was before, for new glibc it will be slightly more costly at startup but will be thread-safe (the usual C++ local static init costs).

[jakubjelinek]

As a minor optimization for the latter you could do in SetAlternateSignalStack() do

const uptr kAltStackSize = GetAltStackSize();
void *base = MmapOrDie(kAltStackSize, __func__);
altstack.ss_sp = (char*) base;
altstack.ss_flags = 0;
altstack.ss_size = kAltStackSize;

[vitalybuka]

Usually I double check that "arc patch" preserves "Author:", however here something went wrong and it's committed under my name.
If you like I can revert/reland under your name.

[eugenis]

I think this happens when the patch is uploaded through web ui and not through arc. It would be great if arc patch made it clear when that is the case!

[serge-sans-paille]

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

168

Interestingly this introduces a dependency on libc++ runtime (call to __cxa_guard_acquire etc for thread safety). @jakubjelinek / @vitalybuka this looks like a regression to me (when linking asan runtime statically for C code). Would that be ok to you ro remove the static (and eventually turn the const into a constexpr but I don't know if SIGSTKSZ can expand to a function call on some platform)

[vitalybuka]

This comment was removed by vitalybuka.

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

 int Atexit(void (*function)(void)) {
 #if !SANITIZER_GO
   return atexit(function);
 #else
   return 0;
 #endif
 }
 
-bool SupportsColoredOutput(fd_t fd) {
-  return isatty(fd) != 0;
-}
+bool SupportsColoredOutput(fd_t fd) { return isatty(fd) != 0; }
 
 #if !SANITIZER_GO
 // TODO(glider): different tools may require different altstack size.
-static const uptr kAltStackSize = SIGSTKSZ * 4;  // SIGSTKSZ is not enough.
+static uptr GetAltStackSize() {
+  // SIGSTKSZ is not enough.
+  static const uptr kAltStackSize = SIGSTKSZ * 4;
+  return kAltStackSize;
+}
 
 void SetAlternateSignalStack() {
   stack_t altstack, oldstack;
   CHECK_EQ(0, sigaltstack(nullptr, &oldstack));
   // If the alternate stack is already in place, do nothing.
   // Android always sets an alternate stack, but it's too small for us.
   if (!SANITIZER_ANDROID && !(oldstack.ss_flags & SS_DISABLE)) return;
   // TODO(glider): the mapped stack should have the MAP_STACK flag in the
   // future. It is not required by man 2 sigaltstack now (they're using
   // malloc()).
-  void* base = MmapOrDie(kAltStackSize, __func__);
+  void *base = MmapOrDie(GetAltStackSize(), __func__);
   altstack.ss_sp = (char*) base;
   altstack.ss_flags = 0;
-  altstack.ss_size = kAltStackSize;
+  altstack.ss_size = GetAltStackSize();
   CHECK_EQ(0, sigaltstack(&altstack, nullptr));
 }
 
 void UnsetAlternateSignalStack() {
   stack_t altstack, oldstack;
   altstack.ss_sp = nullptr;
   altstack.ss_flags = SS_DISABLE;
-  altstack.ss_size = kAltStackSize;  // Some sane value required on Darwin.
+  altstack.ss_size = GetAltStackSize();  // Some sane value required on Darwin.
   CHECK_EQ(0, sigaltstack(&altstack, &oldstack));
   UnmapOrDie(oldstack.ss_sp, oldstack.ss_size);
 }
 
 static void MaybeInstallSigaction(int signum,
                                   SignalHandlerType handler) {
   if (GetHandleSignalMode(signum) == kHandleSignalNo) return;