This is an archive of the discontinued LLVM Phabricator instance.

Differential D42389

[ubsan] Add preinit initializer for ubsan
ClosedPublic

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

Details

Reviewers
vitalybuka
eugenis
pcc
Commits
rG564f845b7483: [ubsan] Add preinit initializer for ubsan
rCRT323249: [ubsan] Add preinit initializer for ubsan
rL323249: [ubsan] Add preinit initializer for ubsan
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 B14095: Diff 130938.Jan 22 2018, 11:48 AM
Harbormaster completed remote builds in B14096: Diff 130939.
vitalybuka added a subscriber: pcc.Jan 22 2018, 12:08 PM

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.

fjricci added a comment.Jan 22 2018, 12:14 PM

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

Harbormaster completed remote builds in B14098: Diff 130949.Jan 22 2018, 12:36 PM
vitalybuka added a comment.Jan 22 2018, 1:01 PM

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.

fjricci added a comment.Jan 22 2018, 1:15 PM

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?

vitalybuka added a comment.Jan 22 2018, 1:36 PM
In D42389#984301, @fjricci wrote:

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

Harbormaster completed remote builds in B14101: Diff 130968.Jan 22 2018, 2:59 PM
smeenai added a subscriber: smeenai.Jan 22 2018, 3:01 PM
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.

fjricci updated this revision to Diff 130984.Jan 22 2018, 4:14 PM

+static

Harbormaster completed remote builds in B14114: Diff 130984.Jan 22 2018, 4:14 PM
Closed by commit rL323249: [ubsan] Add preinit initializer for ubsan (authored by fjricci). · Explain WhyJan 23 2018, 11:56 AM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: hintonda. · View Herald TranscriptJan 23 2018, 11:56 AM

Revision Contents

PathSize
compiler-rt/
trunk/
lib/
ubsan/
1 line
13 lines
36 lines

Diff 131123

compiler-rt/trunk/lib/ubsan/CMakeLists.txt

Show First 20 Lines • Show All 145 Lines • ▼ Show 20 Lines add_compiler_rt_object_libraries(RTUbsan_standalone
ARCHS ${UBSAN_SUPPORTED_ARCH} ARCHS ${UBSAN_SUPPORTED_ARCH}
SOURCES ${UBSAN_STANDALONE_SOURCES} SOURCES ${UBSAN_STANDALONE_SOURCES}
CFLAGS ${UBSAN_STANDALONE_CFLAGS}) CFLAGS ${UBSAN_STANDALONE_CFLAGS})
# Standalone UBSan runtimes. # Standalone UBSan runtimes.
add_compiler_rt_runtime(clang_rt.ubsan_standalone add_compiler_rt_runtime(clang_rt.ubsan_standalone
STATIC STATIC
ARCHS ${UBSAN_SUPPORTED_ARCH} ARCHS ${UBSAN_SUPPORTED_ARCH}
SOURCES ubsan_init_standalone_preinit.cc
OBJECT_LIBS RTSanitizerCommon OBJECT_LIBS RTSanitizerCommon
RTSanitizerCommonLibc RTSanitizerCommonLibc
RTUbsan RTUbsan
RTUbsan_standalone RTUbsan_standalone
RTInterception RTInterception
CFLAGS ${UBSAN_CFLAGS} CFLAGS ${UBSAN_CFLAGS}
PARENT_TARGET ubsan) PARENT_TARGET ubsan)
Show All 35 Lines

compiler-rt/trunk/lib/ubsan/ubsan_flags.cc

Show All 20 Lines
#include <stdlib.h> #include <stdlib.h>
namespace __ubsan { namespace __ubsan {
const char *MaybeCallUbsanDefaultOptions() { const char *MaybeCallUbsanDefaultOptions() {
return (&__ubsan_default_options) ? __ubsan_default_options() : ""; return (&__ubsan_default_options) ? __ubsan_default_options() : "";
} }
static const char *GetFlag(const char *flag) {
// We cannot call getenv() from inside a preinit array initializer
if (SANITIZER_CAN_USE_PREINIT_ARRAY) {
return GetEnv(flag);
} else {
return getenv(flag);
}
}
Flags ubsan_flags; Flags ubsan_flags;
void Flags::SetDefaults() { void Flags::SetDefaults() {
#define UBSAN_FLAG(Type, Name, DefaultValue, Description) Name = DefaultValue; #define UBSAN_FLAG(Type, Name, DefaultValue, Description) Name = DefaultValue;
#include "ubsan_flags.inc" #include "ubsan_flags.inc"
#undef UBSAN_FLAG #undef UBSAN_FLAG
} }
void RegisterUbsanFlags(FlagParser *parser, Flags *f) { void RegisterUbsanFlags(FlagParser *parser, Flags *f) {
#define UBSAN_FLAG(Type, Name, DefaultValue, Description) \ #define UBSAN_FLAG(Type, Name, DefaultValue, Description) \
RegisterFlag(parser, #Name, Description, &f->Name); RegisterFlag(parser, #Name, Description, &f->Name);
#include "ubsan_flags.inc" #include "ubsan_flags.inc"
#undef UBSAN_FLAG #undef UBSAN_FLAG
} }
void InitializeFlags() { void InitializeFlags() {
SetCommonFlagsDefaults(); SetCommonFlagsDefaults();
{ {
CommonFlags cf; CommonFlags cf;
cf.CopyFrom(*common_flags()); cf.CopyFrom(*common_flags());
cf.print_summary = false; cf.print_summary = false;
cf.external_symbolizer_path = getenv("UBSAN_SYMBOLIZER_PATH"); cf.external_symbolizer_path = GetFlag("UBSAN_SYMBOLIZER_PATH");
OverrideCommonFlags(cf); OverrideCommonFlags(cf);
} }
Flags *f = flags(); Flags *f = flags();
f->SetDefaults(); f->SetDefaults();
FlagParser parser; FlagParser parser;
RegisterCommonFlags(&parser); RegisterCommonFlags(&parser);
RegisterUbsanFlags(&parser, f); RegisterUbsanFlags(&parser, f);
// Override from user-specified string. // Override from user-specified string.
parser.ParseString(MaybeCallUbsanDefaultOptions()); parser.ParseString(MaybeCallUbsanDefaultOptions());
// Override from environment variable. // Override from environment variable.
parser.ParseString(getenv("UBSAN_OPTIONS")); parser.ParseString(GetFlag("UBSAN_OPTIONS"));
InitializeCommonFlags(); InitializeCommonFlags();
if (Verbosity()) ReportUnrecognizedFlags(); if (Verbosity()) ReportUnrecognizedFlags();
if (common_flags()->help) parser.PrintFlagDescriptions(); if (common_flags()->help) parser.PrintFlagDescriptions();
} }
} // namespace __ubsan } // namespace __ubsan
SANITIZER_INTERFACE_WEAK_DEF(const char *, __ubsan_default_options, void) { SANITIZER_INTERFACE_WEAK_DEF(const char *, __ubsan_default_options, void) {
return ""; return "";
} }
#endif // CAN_SANITIZE_UB #endif // CAN_SANITIZE_UB

compiler-rt/trunk/lib/ubsan/ubsan_init_standalone_preinit.cc

//===-- ubsan_init_standalone_preinit.cc ---------------------------------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
//
// Initialization of standalone UBSan runtime.
//
//===----------------------------------------------------------------------===//
#include "ubsan_platform.h"
#if !CAN_SANITIZE_UB
#error "UBSan is not supported on this platform!"
#endif
#include "sanitizer_common/sanitizer_internal_defs.h"
#include "ubsan_init.h"
#include "ubsan_signals_standalone.h"
#if SANITIZER_CAN_USE_PREINIT_ARRAY
namespace __ubsan {
static void PreInitAsStandalone() {
InitAsStandalone();
InitializeDeadlySignals();
}
} // namespace __ubsan
__attribute__((section(".preinit_array"), used)) void (*__local_ubsan_preinit)(
void) = __ubsan::PreInitAsStandalone;
#endif // SANITIZER_CAN_USE_PREINIT_ARRAY