Isolate v8 symbols #179
Merged
Isolate v8 symbols #179
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lloeki
force-pushed
the
fix/isolate-v8-symbols
branch
from
858a10e to
75de2be
Compare
SamSaffron
reviewed
Oct 6, 2020
|
Overall I like this, is there any impact to load time of the library? Additionally are we blocked here on a libv8 change? Is there a PR we need to amend it? |
lloeki
force-pushed
the
fix/isolate-v8-symbols
branch
from
75de2be to
1aa06eb
Compare
With I'm looking into the CI break. We've had this working for a while on Linux (including with musl) so I'm not yet sure what the issue is:
With the loader, we're not. In the future, if (when?) libv8 manages to be built with
I'm currently trying to find a simple way to have |
SamSaffron
reviewed
Oct 7, 2020
This Dockerfile makes use of caching and invalidation to save time
between retries.
This drops you in a IRB shell with mini_racer required already:
docker build -t mini_racer .
docker run --rm -it mini_racer
A single command for repeated runs:
docker build -t mini_racer . && \
docker run --rm -it mini_racer bundle exec rake test
It's possible to specify the Ruby version to build upon:
docker build --build-arg RUBY_VERSION=2.6 -t mini_racer . && \
docker run --rm -it mini_racer bundle exec rake test
|
Seems like the issue with
https://travis-ci.org/github/rubyjs/mini_racer/jobs/714390184 https://travis-ci.org/github/rubyjs/mini_racer/jobs/732159781 |
|
I took on myself to add a |
|
There has been a lot complaints over the past few months regarding mini_racer builds failing on old versions of gcc. I strongly suspect this is all somewhat related. |
|
I am fine to merge this, but can you confirm this is going to work on MacOS as well (I can see you allowed for it) Another huge one I would love some help on is: https://chromium-review.googlesource.com/c/v8/v8/+/2416501 This will be enormously useful (especially for sqreen) cause if we have a switch for single threaded mode you could use it and not need to worry about processes forking anymore and breaking the runtime. Would you like to take lead on this? V8 team are waiting on feedback. |
Is this tied to this issue and that commit?
As it turns out I just happen to have a customer who's using the I cannot commit on a timeline but this definitely looks worth a shot. I'll bring this up to the team. |
Yes, this works on macOS (incidentally I work on macOS first and test on Linux later). |
|
Cool, let try this out then! Thanks @lloeki . |
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
May 6, 2022
When Ruby loads a native extension (using `require`), it uses `dlopen(..., RTLD_LAZY | RTLD_GLOBAL)` (https://github.com/ruby/ruby/blob/67950a4c0a884bdb78d9beb4405ebf7459229b21/dln.c#L362). This means that every symbol exposed directly or indirectly by that native extension becomes visible to every other extension in the Ruby process. This can cause issues, see rubyjs/mini_racer#179. Ruby's extension loading mechanism is not configurable -- there's no way to tell it to use different flags when calling `dlopen`. To get around this, this commit introduces introduces another extension (profiling loader) which has only a single responsibility: mimic Ruby's extension loading mechanism, but when calling `dlopen` use a different set of flags. This idea was shamelessly stolen from @lloeki's work in rubyjs/mini_racer#179, big thanks! Note that, that we know of, the profiling native extension only exposes one potentially problematic symbol: `rust_eh_personality` (coming from libddprof/libdatadog). Future versions of Rust have been patched not to expose this (see rust-lang/rust#95604 (comment)) so we may want to revisit the need for this loader in the future, and perhaps delete it if we no longer require its services :) Testing the `ddtrace_profiling_loader.c`'s failure handling paths is a bit non-trivial, so here's how I did it manually (some logs abridged): ```bash # 1️⃣ Validate everything's working fine: $ ruby -v ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-darwin20] $ bundle rake clean compile ... $ DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e 'nil' D, [2022-05-06T12:17:41.554770 #19686] DEBUG -- ddtrace: [ddtrace] (/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/core/configuration/components.rb:340:in `startup!') Profiling started' # 2️⃣ Force library load failure: # (Delete the library version for the Ruby I'm using) $ rm lib/ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20.bundle $ DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e 'nil' W, [2022-05-06T12:19:23.891711 #21159] WARN -- ddtrace: [ddtrace] (/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/core/configuration/components.rb:345:in `startup!') Profiling was requested but is not supported, profiling disabled: There was an error loading the profiling native extension due to 'Failure to load ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20 due to dlopen(/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/profiling/../../ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20.bundle, 5): image not found' at '/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/profiling/load_native_extension.rb:22:in `<top (required)>'' # 3️⃣ Force a library incompatibility: # (Copy library compiled with a different Ruby version) $ cp lib/ddtrace_profiling_native_extension.3.2.0_x86_64-darwin20.bundle lib/ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20.bundle $ DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e 'nil' W, [2022-05-06T12:22:24.036956 #21773] WARN -- ddtrace: [ddtrace] (/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/core/configuration/components.rb:345:in `startup!') Profiling was requested but is not supported, profiling disabled: There was an error loading the profiling native extension due to Failure to load ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20 due to library was compiled and linked to a different Ruby version' at '/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/profiling/load_native_extension.rb:22:in `<top (required)>' # 4️⃣ Force an initialization failure: # (Restore library clobbered in previous steps) $ bundle rake clean compile ... # (Rename expected initialization function) $ git diff -U0 lib/datadog/profiling/load_native_extension.rb diff --git a/lib/datadog/profiling/load_native_extension.rb b/lib/datadog/profiling/load_native_extension.rb index 5ea4bb32c..af964bade 100644 --- a/lib/datadog/profiling/load_native_extension.rb +++ b/lib/datadog/profiling/load_native_extension.rb @@ -18 +18 @@ full_file_path = "#{__dir__}/../../#{extension_name}.#{RbConfig::CONFIG['DLEXT'] -init_function_name = "Init_#{extension_name.split('.').first}" +init_function_name = "Init_#{extension_name.split('.').first}_kaboom $ DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e 'nil' W, [2022-05-06T12:27:55.706221 #23036] WARN -- ddtrace: [ddtrace] (/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/core/configuration/components.rb:345:in `startup!') Profiling was requested but is not supported, profiling disabled: There was an error loading the profiling native extension due to 'Failure to load ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20 due to dlsym(0x7fe144151630, Init_ddtrace_profiling_native_extension_kaboom): symbol not found' at '/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/profiling/load_native_extension.rb:22:in `<top (required)>'' ```
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
May 6, 2022
When Ruby loads a native extension (using `require`), it uses `dlopen(..., RTLD_LAZY | RTLD_GLOBAL)` (https://github.com/ruby/ruby/blob/67950a4c0a884bdb78d9beb4405ebf7459229b21/dln.c#L362). This means that every symbol exposed directly or indirectly by that native extension becomes visible to every other extension in the Ruby process. This can cause issues, see rubyjs/mini_racer#179. Ruby's extension loading mechanism is not configurable -- there's no way to tell it to use different flags when calling `dlopen`. To get around this, this commit introduces introduces another extension (profiling loader) which has only a single responsibility: mimic Ruby's extension loading mechanism, but when calling `dlopen` use a different set of flags. This idea was shamelessly stolen from @lloeki's work in rubyjs/mini_racer#179, big thanks! Note that, that we know of, the profiling native extension only exposes one potentially problematic symbol: `rust_eh_personality` (coming from libddprof/libdatadog). Future versions of Rust have been patched not to expose this (see rust-lang/rust#95604 (comment)) so we may want to revisit the need for this loader in the future, and perhaps delete it if we no longer require its services :) Testing the `ddtrace_profiling_loader.c`'s failure handling paths is a bit non-trivial, so here's how I did it manually (some logs abridged): ```bash # 1️⃣ Validate everything's working fine: $ ruby -v ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-darwin20] $ bundle rake clean compile ... $ DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e 'nil' D, [2022-05-06T12:17:41.554770 #19686] DEBUG -- ddtrace: [ddtrace] (/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/core/configuration/components.rb:340:in `startup!') Profiling started' # 2️⃣ Force library load failure: # (Delete the library version for the Ruby I'm using) $ rm lib/ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20.bundle $ DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e 'nil' W, [2022-05-06T12:19:23.891711 #21159] WARN -- ddtrace: [ddtrace] (/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/core/configuration/components.rb:345:in `startup!') Profiling was requested but is not supported, profiling disabled: There was an error loading the profiling native extension due to 'Failure to load ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20 due to dlopen(/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/profiling/../../ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20.bundle, 5): image not found' at '/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/profiling/load_native_extension.rb:22:in `<top (required)>'' # 3️⃣ Force a library incompatibility: # (Copy library compiled with a different Ruby version) $ cp lib/ddtrace_profiling_native_extension.3.2.0_x86_64-darwin20.bundle lib/ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20.bundle $ DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e 'nil' W, [2022-05-06T12:22:24.036956 #21773] WARN -- ddtrace: [ddtrace] (/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/core/configuration/components.rb:345:in `startup!') Profiling was requested but is not supported, profiling disabled: There was an error loading the profiling native extension due to Failure to load ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20 due to library was compiled and linked to a different Ruby version' at '/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/profiling/load_native_extension.rb:22:in `<top (required)>' # 4️⃣ Force an initialization failure: # (Restore library clobbered in previous steps) $ bundle rake clean compile ... # (Rename expected initialization function) $ git diff -U0 lib/datadog/profiling/load_native_extension.rb diff --git a/lib/datadog/profiling/load_native_extension.rb b/lib/datadog/profiling/load_native_extension.rb index 5ea4bb32c..af964bade 100644 --- a/lib/datadog/profiling/load_native_extension.rb +++ b/lib/datadog/profiling/load_native_extension.rb @@ -18 +18 @@ full_file_path = "#{__dir__}/../../#{extension_name}.#{RbConfig::CONFIG['DLEXT'] -init_function_name = "Init_#{extension_name.split('.').first}" +init_function_name = "Init_#{extension_name.split('.').first}_kaboom $ DD_TRACE_DEBUG=true DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e 'nil' W, [2022-05-06T12:27:55.706221 #23036] WARN -- ddtrace: [ddtrace] (/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/core/configuration/components.rb:345:in `startup!') Profiling was requested but is not supported, profiling disabled: There was an error loading the profiling native extension due to 'Failure to load ddtrace_profiling_native_extension.2.7.6_x86_64-darwin20 due to dlsym(0x7fe144151630, Init_ddtrace_profiling_native_extension_kaboom): symbol not found' at '/Users/ivo.anjo/datadog/dd-trace-rb/lib/datadog/profiling/load_native_extension.rb:22:in `<top (required)>'' ```
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
Feb 6, 2025
**What does this PR do?** This PR removes the `datadog_profiling_loader` native extension, as it's no longer needed. The profiling native loader was added in #2003 . Quoting from that PR's description: > When Ruby loads a native extension (using `require`), it uses > `dlopen(..., RTLD_LAZY | RTLD_GLOBAL)` > (https://github.com/ruby/ruby/blob/67950a4c0a884bdb78d9beb4405ebf7459229b21/dln.c#L362). > > This means that every symbol exposed directly or indirectly by that > native extension becomes visible to every other extension in the > Ruby process. This can cause issues, see > rubyjs/mini_racer#179 . > > Ruby's extension loading mechanism is not configurable -- there's > no way to tell it to use different flags when calling `dlopen`. > To get around this, this commit introduces introduces another > extension (profiling loader) which has only a single responsibility: > mimic Ruby's extension loading mechanism, but when calling > `dlopen` use a different set of flags. > > This idea was shamelessly stolen from @lloeki's work in > rubyjs/mini_racer#179, big thanks! ...and importantly ends with: > Note that, that we know of, the profiling native extension only > exposes one potentially problematic symbol: > `rust_eh_personality` (coming from libddprof/libdatadog). > > Future versions of Rust have been patched not to expose this > (see rust-lang/rust#95604 (comment)) > so we may want to revisit the need for this loader in the future, > and perhaps delete it if we no longer require its services :) And we have reached the situation predicted in that description: 1. Nowadays libdatadog no longer exposes `rust_eh_personality` 2. We have a test that validates that only expected symbols are exported by the libdatadog library (see DataDog/libdatadog#573 ). Any new symbols that show up would break shipping new libdatadog versions to rubygems.org until we review them. 3. The `libdatadog_api` extension, which we've been shipping for customers since release 2.3.0 back in July 2024 has always been loaded directly without a loader without issues. Thus, I think it's the right time to get rid of the loader. **Motivation:** Having the loader around is not zero cost; we've run into/caused a few issues ( #2250 and #3582 come to mind). It also adds overhead to development: every time we need to rebuild the extensions, it's an extra extension that needs to be prepared, rebuilt, copied, etc. I've been meaning to get rid of the loader for some time now, and this came up again this week so I decided it was time to do it. **Additional Notes:** In the future, we can always ressurrect this approach if we figure out we need it again. We've also discussed internally about proposing upstream to the Ruby VM to maybe add an API to do what the loader was doing, so that we wouldn't need the weird workaround. We haven't yet decided if we're going to do that (help welcome!). **How to test the change?** The loader was responsible for loading the rest of profiling. Thus, any test that uses profiling was also validating the loader and now will validate that we're doing fine without it. TL;DR green CI is good.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello!
We identified an issue happening when two extensions are built with different V8 versions, then loaded at the same time.
In an ideal world, all extensions would be built with
-fvisibility=hidden, only exposing relevant symbols and avoiding clases, unfortunately that is not the case.Indeed the libv8 gem does not build its static library with
-fvisibility=hidden. The result is that when this static library is linked into a Ruby extension (IOW a dynamic library), V8 symbols are global, irrespective of passing-fvisibility=hiddenwhen building the extension.Additionally, Ruby doesn't really make an effort to load its extensions in isolated ways, resulting in the default of globally available symbols.
As a result, the second extension being loaded will resolve some of its V8 symbols to the first ones that were loaded.
To combat this, within Sqreen's fork of MiniRacer, we created a small wrapper that takes in charge loading the extension more privately by:
RTLD_LOCAL), preventing other libs to pick its symbols upRTLD_DEEPBIND), preventing this lib to pick others symbolsUnfortunately, the latter is not always available, resulting in MiniRacer's V8 symbols being picked up by other libraries, even when they use
RTLD_LOCAL(which only controls things one way). In that case we are left with no option.This PR attempts to make MiniRacer a good citizen in two ways:
-fvisibility=hiddento hide any irrelevantmini_racer_extensionsymbols, and be ready if/whenlibv8_monolith.ais compiled with it too.libv8_monolith.athat wasn't built with-fvisibility=hidden.Considered alternatives
ld-exported_symbols_listand-unexported_symbols_list, but it needs exhaustive generation of symbols to hide and show. Possible, but calling intonm libv8_monolith.aand parsing the output to generate the list could be unreliable.libv8version and build: unfortunately this does not mean a given lib wasn't built with a different v8, unless all gems pin theirlibv8versions exactly in theirgemspec(this particular one stumped us for a long time)Excerpt of
mini_racer_extension.bundlesymbol table (Tmeans global,tmeans local`):Excerpt of a stack trace where code jumps from one lib to the other within libv8: