This is an archive of the discontinued LLVM Phabricator instance.

Sanitizer built against glibc 2.34 doesn't work

Authored by jakubjelinek on Apr 16 2021, 6:39 AM.



As mentioned in , glibc starting with the;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.

Diff Detail

Event Timeline

jakubjelinek requested review of this revision.Apr 16 2021, 6:39 AM
jakubjelinek created this revision.
Herald added a subscriber: Restricted Project. · View Herald TranscriptApr 16 2021, 6:39 AM
marxin added a subscriber: marxin.Apr 16 2021, 6:54 AM

I do support the suggested change.

vitalybuka added inline comments.

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

vitalybuka accepted this revision.Apr 16 2021, 9:47 AM
vitalybuka added a reviewer: vitalybuka.

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

This revision is now accepted and ready to land.Apr 16 2021, 9:48 AM

undo unrelated change

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

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;
This revision was landed with ongoing or failed builds.Apr 16 2021, 11:21 AM
This revision was automatically updated to reflect the committed changes.

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.

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!

Harbormaster completed remote builds in B99211: Diff 338163.
serge-sans-paille added inline comments.

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)

This comment was removed by vitalybuka.