Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(608)

Issue 1111373003: crypto: Load libchaps.so with RTLD_DEEPBIND (Closed)

Created:
5 years, 7 months ago by spang
Modified:
5 years, 7 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

crypto: Load libchaps.so with RTLD_DEEPBIND This fixes the component=shared_library build on Chrome OS. Previously, the TPM token initialization would crash the browser because libchaps.so loads a 2nd, incompatible copy of the protobuf-lite library into the browser. The crash happens because by default the global scope takes precedence, so symbols in the bundled copy shadow symbols in the system copy. This can lead to accessing the wrong object or calling the wrong function. The usual symptom is a crash during static initialization of libchaps.so. RTL_DEEPBIND rearranges the scope for libchaps.so so that the system library takes precedence instead. The scope for chrome is unaffected. Some other possibilities were considered: - Unbundling the library. This would cause us to lose our local modifications to libprotobuf on Chrome OS. - Fixing the soname so that the bundled copy is used by chaps. This doesn't even link because the bundled code is not ABI compatible with upstream. - Statically linking libchaps.so. This is tricky because we need position-independent static libs for its deps, and we don't support building those currently. It will also cause bloat. This hack is minimally invasive and allows use of shared library builds on Chrome OS devices. BUG=175508 TEST=cros chrome-sdk --board=link GYP_DEFINES="$GYP_DEFINES component=shared_library" gclient runhooks ninja -C out_link/Release chrome chrome-sandbox deploy_chrome --board link --build-dir out_link/Release --to $IP Log into the system. No crash. TPM-backed user certs show up in chrome://certificate-manager. Committed: https://crrev.com/ba254c4e4a69cfa48871ab483fd3dc9c98b3582c Cr-Commit-Position: refs/heads/master@{#329743}

Patch Set 1 #

Patch Set 2 : move inside if (!tpm_args->chaps_module) {} #

Patch Set 3 : move to class #

Total comments: 1

Patch Set 4 : s/Drop ours.// #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -0 lines) Patch
M crypto/nss_util.cc View 1 2 3 3 chunks +38 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (3 generated)
spang
The shared_library build has been broken on Chrome OS for at least 2 years. I'd ...
5 years, 7 months ago (2015-04-29 19:28:38 UTC) #2
Darren Krahn
On 2015/04/29 19:28:38, spang wrote: > The shared_library build has been broken on Chrome OS ...
5 years, 7 months ago (2015-04-30 00:05:45 UTC) #3
Ryan Sleevi
On 2015/04/30 00:05:45, Darren Krahn wrote: > Does RTLD_DEEPBIND have side effects that would be ...
5 years, 7 months ago (2015-04-30 00:21:16 UTC) #4
spang
On 2015/04/30 00:21:16, Ryan Sleevi wrote: > On 2015/04/30 00:05:45, Darren Krahn wrote: > > ...
5 years, 7 months ago (2015-04-30 01:02:18 UTC) #5
Ryan Sleevi
On 2015/04/30 01:02:18, spang wrote: > It's not the PKCS#11 module causing the conflict, but ...
5 years, 7 months ago (2015-04-30 01:04:36 UTC) #6
spang
On 2015/04/30 01:04:36, Ryan Sleevi wrote: > On 2015/04/30 01:02:18, spang wrote: > > It's ...
5 years, 7 months ago (2015-04-30 01:06:50 UTC) #7
spang
On 2015/04/30 01:06:50, spang wrote: > On 2015/04/30 01:04:36, Ryan Sleevi wrote: > > On ...
5 years, 7 months ago (2015-05-04 18:42:20 UTC) #8
Ryan Sleevi
On 2015/05/04 18:42:20, spang wrote: > Should we go with that approach? There will obviously ...
5 years, 7 months ago (2015-05-05 01:10:10 UTC) #9
Darren Krahn
On 2015/05/05 01:10:10, Ryan Sleevi wrote: > On 2015/05/04 18:42:20, spang wrote: > > Should ...
5 years, 7 months ago (2015-05-05 17:54:22 UTC) #10
spang
On 2015/05/05 17:54:22, Darren Krahn wrote: > On 2015/05/05 01:10:10, Ryan Sleevi wrote: > > ...
5 years, 7 months ago (2015-05-12 20:59:30 UTC) #11
Darren Krahn
On 2015/05/12 20:59:30, spang wrote: > On 2015/05/05 17:54:22, Darren Krahn wrote: > > On ...
5 years, 7 months ago (2015-05-12 21:18:35 UTC) #12
Ryan Sleevi
I'm really not keen for hacks, which this feels like, certainly in approach. This would ...
5 years, 7 months ago (2015-05-12 21:31:56 UTC) #13
spang
On 2015/05/12 21:31:56, Ryan Sleevi wrote: > I'm really not keen for hacks, which this ...
5 years, 7 months ago (2015-05-12 21:45:07 UTC) #14
davidben
This sounds similar to some BoringSSL/system-OpenSSL issues I was worried we might start having if ...
5 years, 7 months ago (2015-05-12 21:46:22 UTC) #15
spang
On 2015/05/12 21:46:22, David Benjamin wrote: > This sounds similar to some BoringSSL/system-OpenSSL issues I ...
5 years, 7 months ago (2015-05-12 22:29:48 UTC) #16
spang
On 2015/05/12 21:31:56, Ryan Sleevi wrote: > I'm really not keen for hacks, which this ...
5 years, 7 months ago (2015-05-13 18:52:26 UTC) #17
Ryan Sleevi
lgtm https://codereview.chromium.org/1111373003/diff/40001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/1111373003/diff/40001/crypto/nss_util.cc#newcode295 crypto/nss_util.cc:295: // LoadModule() will have taken a 2nd reference. ...
5 years, 7 months ago (2015-05-13 22:35:52 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1111373003/60001
5 years, 7 months ago (2015-05-13 22:38:38 UTC) #21
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 7 months ago (2015-05-14 00:00:32 UTC) #22
commit-bot: I haz the power
5 years, 7 months ago (2015-05-14 00:02:19 UTC) #23
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/ba254c4e4a69cfa48871ab483fd3dc9c98b3582c
Cr-Commit-Position: refs/heads/master@{#329743}

Powered by Google App Engine
This is Rietveld 408576698