Page MenuHomePhabricator

ubsan: Allow programs to use setenv to configure ubsan_standalone.
ClosedPublic

Authored by pcc on Nov 8 2017, 5:40 PM.

Details

Summary

Previously ubsan_standalone used the GetEnv function to read the
environment variables UBSAN_OPTIONS and UBSAN_SYMBOLIZER_PATH. The
problem with GetEnv is that it does not respect changes to the
environment variables made using the libc setenv function, which
prevents clients from setting environment variables to configure
ubsan before loading ubsan-instrumented libraries.

The reason why we have GetEnv is that some runtimes need to read
environment variables while they initialize using .preinit_array,
and getenv does not work while .preinit_array functions are being
called. However, it is unnecessary for ubsan_standalone to initialize
that early. So this change switches ubsan_standalone to using getenv
and removes the .preinit_array entry. The static version of the runtime
still ends up being initialized using a C++ constructor that exists
to support the shared runtime.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Nov 8 2017, 5:40 PM
vitalybuka edited edge metadata.Nov 8 2017, 5:49 PM

we have interceptors for nicer support of deadly signals, so probably we'd like to install them ASAP
also there is weak __ubsan_default_options() which can be used instead of setenv

pcc added a comment.Nov 8 2017, 6:09 PM

we have interceptors for nicer support of deadly signals, so probably we'd like to install them ASAP

I'm not sure if that is too important, especially since a user has the choice of using (e.g.) ubsan+asan if they want early interception of signals.

also there is weak __ubsan_default_options() which can be used instead of setenv

That won't always work. I'm thinking about the Android app use case where you may have multiple ubsan-instrumented libraries and a main program written in another language that wants to configure the runtime.

it was added here https://reviews.llvm.org/rL213983

I found that change, but it does not explain why ubsan would want .preinit_array specifically instead of .init_array (aside from consistency with other sanitizers).

vitalybuka accepted this revision.Nov 8 2017, 6:13 PM

Then I don't have anything particular against this patch.

This revision is now accepted and ready to land.Nov 8 2017, 6:13 PM
This revision was automatically updated to reflect the committed changes.