This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] Add preinit initializer for ubsan
ClosedPublic

Authored by fjricci on Jan 22 2018, 11:47 AM.

Details

Summary

Now that ubsan does function interception (for signals), we
need to ensure that ubsan is initialized before any library
constructors are called. Otherwise, if a constructor calls
sigaction, ubsan will intercept in an unitialized state, which
will cause a crash.

This patch is a partial revert of r317757, which removed
preinit arrays for ubsan.

Diff Detail

Repository
rL LLVM

Event Timeline

fjricci created this revision.Jan 22 2018, 11:47 AM
Herald added subscribers: Restricted Project, mgorny, kubamracek. · View Herald TranscriptJan 22 2018, 11:47 AM
fjricci updated this revision to Diff 130939.Jan 22 2018, 11:48 AM

Remove extra whitespace

Harbormaster completed remote builds in B14096: Diff 130939.

It was disabled by @pcc in D39827 because of Android issues, and now D42329 disabled signal handling on Android at all, So I guess it's safe to return preinit arrays.

lib/ubsan/CMakeLists.txt
14 ↗(On Diff #130939)

This goes into both static and shared, but must go into STATIC only. Don't remember details but it will fails on Android and Apple bots.
PTAL at D39827. Maybe just restore relevant parts of D39827.

lib/ubsan/ubsan_init.h
34 ↗(On Diff #130939)

I am not sure that we need this in API, I'd prefer D39827 approach.

Great, thanks, I'll pull stuff in from that patch.

SANITIZER_CAN_USE_PREINIT_ARRAY is set to false when SANITIZER_ANDROID is defined, so we should be fine on the android front.

fjricci updated this revision to Diff 130949.Jan 22 2018, 12:35 PM
fjricci edited the summary of this revision. (Show Details)
fjricci added a reviewer: pcc.
fjricci removed a subscriber: pcc.

incorporate D39827

Oh, reverting env part of D39827 is necessary.
We can't use getenv() from preinit array.
@pcc If we revert getenv() part, we would not be able to use getenv/setenv for sanitizer options. but I guess we don't need them here as we don't need them for any other sanitizers.

pcc added a comment.Jan 22 2018, 1:08 PM

@pcc If we revert getenv() part, we would not be able to use getenv/setenv for sanitizer options. but I guess we don't need them here as we don't need them for any other sanitizers.

No, we need it to support configuring the runtime using setenv for the reasons described in the review of D39827.

No, we need it to support configuring the runtime using setenv for the reasons described in the review of D39827.

Afaik, those reasons are android app-specific. We could theoretically still use getenv() on android, since preinit arrays aren't supported there anyway.

I guess we don't need them here as we don't need them for any other sanitizers.

Are there any other sanitizers that we link in statically on android? That could be the difference?

No, we need it to support configuring the runtime using setenv for the reasons described in the review of D39827.

Afaik, those reasons are android app-specific. We could theoretically still use getenv() on android, since preinit arrays aren't supported there anyway.

Could you update your patch to pick getenv vs GetEnv depending on SANITIZER_CAN_USE_PREINIT_ARRAY.

fjricci updated this revision to Diff 130968.Jan 22 2018, 2:58 PM

Don't use getenv when using the preinit array

vitalybuka accepted this revision.Jan 22 2018, 3:36 PM
vitalybuka added inline comments.
lib/ubsan/ubsan_flags.cc
29 ↗(On Diff #130968)

static here and above?

This revision is now accepted and ready to land.Jan 22 2018, 3:36 PM
fjricci added inline comments.Jan 22 2018, 4:08 PM
lib/ubsan/ubsan_flags.cc
29 ↗(On Diff #130968)

MaybeCallUbsanDefaultOptions is declared extern, but I'll make GetFlag static.

This revision was automatically updated to reflect the committed changes.