Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1437)

Issue 2277863002: Enable loading native libraries with RTLD_DEEPBIND (Closed)

Created:
3 years, 10 months ago by Ken Rockot(use gerrit already)
Modified:
3 years, 10 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. This CL adds a NativeLibraryOptions structure and a new LoadNativeLibraryWithOptions() call. The only supported option (|prefer_own_symbols|) only affects behavior on non-OSX, non-Android, POSIX systems. The default behavior of LoadNativeLibrary is unchanged. Also adds a unit test for general native library loading since there wasn't one before. BUG=594674 Committed: https://crrev.com/596a0dd73a3b8f8eaacb58526540107efcba5a57 Cr-Commit-Position: refs/heads/master@{#414605}

Patch Set 1 : . #

Total comments: 20

Patch Set 2 : . #

Total comments: 3

Patch Set 3 : Enable loading native libraries with RTLD_DEEPBIND #

Total comments: 1

Patch Set 4 : . #

Total comments: 7

Patch Set 5 : \o/ #

Total comments: 6

Patch Set 6 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+264 lines, -11 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 4 chunks +7 lines, -0 lines 0 comments Download
M base/native_library.h View 1 2 3 4 1 chunk +20 lines, -0 lines 0 comments Download
A base/native_library.cc View 1 chunk +15 lines, -0 lines 0 comments Download
M base/native_library_ios.mm View 1 chunk +3 lines, -2 lines 0 comments Download
M base/native_library_mac.mm View 1 chunk +3 lines, -2 lines 0 comments Download
M base/native_library_posix.cc View 1 2 3 4 1 chunk +16 lines, -5 lines 0 comments Download
M base/native_library_unittest.cc View 1 2 3 4 5 2 chunks +100 lines, -0 lines 0 comments Download
M base/native_library_win.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M base/test/BUILD.gn View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A base/test/native_library_test_utils.h View 1 2 3 4 1 chunk +26 lines, -0 lines 0 comments Download
A base/test/native_library_test_utils.cc View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
A base/test/test_shared_library.cc View 1 2 3 4 1 chunk +30 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 78 (56 generated)
Ken Rockot(use gerrit already)
thestig@, please take a look - see bug for context. Thanks! +primiano@ +sky@ FYI
3 years, 10 months ago (2016-08-24 19:41:34 UTC) #12
sky
I'm not an owner here. Thanks for figuring this out! https://codereview.chromium.org/2277863002/diff/60001/base/native_library.h File base/native_library.h (right): https://codereview.chromium.org/2277863002/diff/60001/base/native_library.h#newcode69 ...
3 years, 10 months ago (2016-08-24 20:28:17 UTC) #17
Lei Zhang
Got red bots. I actually haven't been paying full attention to the bug, so I ...
3 years, 10 months ago (2016-08-24 20:35:43 UTC) #18
Ken Rockot(use gerrit already)
On 2016/08/24 at 20:28:17, sky wrote: > I'm not an owner here. Thanks for figuring ...
3 years, 10 months ago (2016-08-24 21:29:27 UTC) #30
Primiano Tucci (use gerrit)
I saw this coming :) Initial pass here, the intent seems good, I'd probably just ...
3 years, 10 months ago (2016-08-24 22:14:09 UTC) #31
Ken Rockot(use gerrit already)
Thanks! Unsurprisingly, ASAN is unhappy with the intentional ODR violation in the unit test. I've ...
3 years, 10 months ago (2016-08-24 23:23:57 UTC) #34
Ken Rockot(use gerrit already)
Alright, well it turns out that I only greened the tests with my use_gold/EXPECT_GLOBAL_RTLD_BEHAVIOR hack ...
3 years, 10 months ago (2016-08-25 00:37:01 UTC) #39
Primiano Tucci (use gerrit)
> Unsurprisingly, ASAN is unhappy with the intentional ODR violation in the unit test. I've ...
3 years, 10 months ago (2016-08-25 10:20:45 UTC) #42
Ken Rockot(use gerrit already)
Thanks! No new changes yet, just some replies On 2016/08/25 at 10:20:45, primiano wrote: > ...
3 years, 10 months ago (2016-08-25 15:52:08 UTC) #43
Ken Rockot(use gerrit already)
https://codereview.chromium.org/2277863002/diff/120001/base/native_library_mac.mm File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_mac.mm#newcode45 base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. On ...
3 years, 10 months ago (2016-08-25 15:52:13 UTC) #44
Primiano Tucci (use gerrit)
https://codereview.chromium.org/2277863002/diff/120001/base/native_library_mac.mm File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_mac.mm#newcode45 base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. On ...
3 years, 10 months ago (2016-08-25 16:14:33 UTC) #45
Ken Rockot(use gerrit already)
On 2016/08/25 at 16:14:33, primiano wrote: > https://codereview.chromium.org/2277863002/diff/120001/base/native_library_mac.mm > File base/native_library_mac.mm (right): > > https://codereview.chromium.org/2277863002/diff/120001/base/native_library_mac.mm#newcode45 ...
3 years, 10 months ago (2016-08-25 16:42:37 UTC) #46
Ken Rockot(use gerrit already)
Reworked the test and added a CHECK for Android https://codereview.chromium.org/2277863002/diff/240001/base/native_library_unittest.cc File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/240001/base/native_library_unittest.cc#newcode126 base/native_library_unittest.cc:126: ...
3 years, 10 months ago (2016-08-25 17:43:21 UTC) #51
Ken Rockot(use gerrit already)
OK! I believe I'm now modeling the correct symbol arrangement to sufficiently test this behavior. ...
3 years, 10 months ago (2016-08-25 20:41:43 UTC) #57
Primiano Tucci (use gerrit)
Excellent, thanks a lot. LGTM with some small final comments (I am not an owner ...
3 years, 10 months ago (2016-08-25 21:10:53 UTC) #60
Ken Rockot(use gerrit already)
https://codereview.chromium.org/2277863002/diff/280001/base/native_library.h File base/native_library.h (right): https://codereview.chromium.org/2277863002/diff/280001/base/native_library.h#newcode69 base/native_library.h:69: NativeLibraryOptions() {} On 2016/08/25 at 21:10:53, Primiano Tucci wrote: ...
3 years, 10 months ago (2016-08-25 21:25:01 UTC) #63
Lei Zhang
lgtm https://codereview.chromium.org/2277863002/diff/300001/base/native_library_mac.mm File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/300001/base/native_library_mac.mm#newcode43 base/native_library_mac.mm:43: const NativeLibraryOptions& options, Do we want to DCHECK() ...
3 years, 10 months ago (2016-08-25 23:08:58 UTC) #66
Ken Rockot(use gerrit already)
Thanks! https://codereview.chromium.org/2277863002/diff/300001/base/native_library_mac.mm File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/300001/base/native_library_mac.mm#newcode43 base/native_library_mac.mm:43: const NativeLibraryOptions& options, On 2016/08/25 at 23:08:58, Lei ...
3 years, 10 months ago (2016-08-25 23:20:43 UTC) #69
Lei Zhang
On 2016/08/25 23:20:43, Ken Rockot wrote: > Indeed, we went over this already :) To ...
3 years, 10 months ago (2016-08-25 23:28:32 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2277863002/320001
3 years, 10 months ago (2016-08-26 00:53:49 UTC) #75
commit-bot: I haz the power
Committed patchset #6 (id:320001)
3 years, 10 months ago (2016-08-26 00:58:08 UTC) #76
commit-bot: I haz the power
3 years, 10 months ago (2016-08-26 00:59:29 UTC) #78
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/596a0dd73a3b8f8eaacb58526540107efcba5a57
Cr-Commit-Position: refs/heads/master@{#414605}

Powered by Google App Engine
This is Rietveld 408576698