Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
962593002:
Use dlsym(RTLD_DEFAULT) instead of dlsym(dlopen(NULL)) in aesgcmchromium.patch. (Closed)
|
Created:
5 years, 10 months ago by ppi Modified:
5 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, blundell, ramant (doing other things) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse dlsym(RTLD_DEFAULT) instead of dlsym(dlopen(NULL)) in aesgcmchromium.patch.
This patch updates the dynamic lookup of NSS symbols to use
dlsym(RTLD_DEFAULT) instead of dlsym on the dlopen(NULL) handle.
The latter doesn't work if the binary that does the dynamic lookup was
loaded dynamically itself (as dlopen(NULL) gives access only to the
symbols loaded at the start of the program).
Note that RTLD_DEFAULT requires setting _GNU_SOURCE.
BUG=462203
Committed: https://crrev.com/f7302e6c7f955f11679ab844cfe3d326f47bb0dc
Cr-Commit-Position: refs/heads/master@{#318863}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Address Ryan's comments. #
Total comments: 1
Patch Set 3 : Merge the patch change into aesgcmchromium.patch #
Total comments: 10
Patch Set 4 : Rebase. #Patch Set 5 : Address Ryan's comments -> fix the .patch. #Messages
Total messages: 19 (4 generated)
ppi@chromium.org changed reviewers: + jamesr@chromium.org, qsr@chromium.org, rsleevi@chromium.org
So it turns out that dlsym(RTLD_DEFAULT) is actually different from dlsym(dlopen(NULL)) and works for us, yay. As discussed on the mailing list, we'll workaround the issue short-term in mojo_shell, but could this work as the proper fix? Any concerns about setting _GNU_SOURCE here?
Rather than tweaking the GYP/GN files, what about as below? https://codereview.chromium.org/962593002/diff/1/net/third_party/nss/ssl/ssl3... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/962593002/diff/1/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3con.c:10: In NSPR, the idiom is to #define _GNU_SOURCE 1 here, before any includes. For example - http://mxr.mozilla.org/nss/source/lib/freebl/stubs.c#10
Thanks Ryan, ptal. BTW I assume that adding a separate .patch here is more reasonable than modifying the existing one, so that we don't need to rewrite all .patches that are applied after it. Wdyt? The /patches overall seem bitrot, I got quite a few rejections when trying to apply them on top of what I got from hg clone https://hg.mozilla.org/projects/nss NSS_3_15_5_BETA2. https://codereview.chromium.org/962593002/diff/20001/net/third_party/nss/patc... File net/third_party/nss/patches/applypatches.sh (right): https://codereview.chromium.org/962593002/diff/20001/net/third_party/nss/patc... net/third_party/nss/patches/applypatches.sh:56: patch -p4 < $patches_dir/removebuildmetadata.patch FYI, this removebuildmetadata.patch seems to be incorrect. It should read -p5, but even then it fails to revert if I do patch -p5 -R < ../patches/removebuildmetadata.patch in net/third_party/nss/ssl
On 2015/02/27 15:24:27, ppi wrote: > Thanks Ryan, ptal. > > BTW I assume that adding a separate .patch here is more reasonable than > modifying the existing one, so that we don't need to rewrite all .patches that > are applied after it. Wdyt? > > The /patches overall seem bitrot, I got quite a few rejections when trying to > apply them on top of what I got from hg clone > https://hg.mozilla.org/projects/nss NSS_3_15_5_BETA2. Could you explain what you meant by bitrot? Perhaps missing https://chromium.googlesource.com/chromium/src/+/c9089fa21c59d9797398e660e494... ? > > https://codereview.chromium.org/962593002/diff/20001/net/third_party/nss/patc... > File net/third_party/nss/patches/applypatches.sh (right): > > https://codereview.chromium.org/962593002/diff/20001/net/third_party/nss/patc... > net/third_party/nss/patches/applypatches.sh:56: patch -p4 < > $patches_dir/removebuildmetadata.patch > FYI, this removebuildmetadata.patch seems to be incorrect. It should read -p5, > but even then it fails to revert if I do > > patch -p5 -R < ../patches/removebuildmetadata.patch in net/third_party/nss/ssl Yes, it appears that one wasn't reviewed thoroughly :/
I would prefer you kept it in the original patch. I am not keen to have patches on our patches in our patch directory.
Question mostly for Ryan: is NSS's soname not something we can depend on? It
seems doing the runtime equivalent of how the linker resolved PK11_Encrypt would
most reliable. (I was actually kind of confused RTLD_DEFAULT works, but okay
then.) Since the linker needs to find NSS by the soname burned into our binary,
I'd think dlopen("thesonameweuse.so") should reliably give you back the right
handle.
Whereas conceivably some other code dlopened another library with RTLD_LOCAL
that also defined a PK11_Encrypt and blegh... (It's quite likely something would
have exploded in that case anyway; Linux's linker is terrible at isolating
library symbol tables from each other. So if this works, I'm fine with it. We're
hoping to make it all go away sooner or later anyway.)
On 2015/02/27 21:37:29, David Benjamin wrote:
> Question mostly for Ryan: is NSS's soname not something we can depend on? It
> seems doing the runtime equivalent of how the linker resolved PK11_Encrypt
would
> most reliable. (I was actually kind of confused RTLD_DEFAULT works, but okay
> then.) Since the linker needs to find NSS by the soname burned into our
binary,
> I'd think dlopen("thesonameweuse.so") should reliably give you back the right
> handle.
>
I don't believe so. Look at the contortions we had to go through just to find
NSS (
https://code.google.com/p/chromium/codesearch#chromium/src/crypto/nss_util.cc...
)
TL;DR: relative path search was/is insufficient.
> Whereas conceivably some other code dlopened another library with RTLD_LOCAL
> that also defined a PK11_Encrypt and blegh... (It's quite likely something
would
> have exploded in that case anyway; Linux's linker is terrible at isolating
> library symbol tables from each other. So if this works, I'm fine with it.
We're
> hoping to make it all go away sooner or later anyway.)
In theory, yes.
In practice, no :)
That's why I find it the better of the two solutions
Ryan, by "bitrot" I meant that when I try to apply the patches on top of the upstream (cloned at the tag referenced in README) I get plenty of rejections. I assume this is because of modifications made to the patches that accidentally broke them (iiuc it's been a while since we rolled NSS). I can squash this change into the existing patch, but not being able to verify that it works and to rewrite the patches that come after aesgcmchromium.patch, I'm afraid of breaking the broken even more. It's perfectly possible that I missed something though and I'm trying to apply them wrong - I will take a closer look at the rejections I get on Monday. Do you find the bitrot hypothesis unlikely?
On 2015/02/27 21:55:37, ppi wrote: > Ryan, by "bitrot" I meant that when I try to apply the patches on top of the > upstream (cloned at the tag referenced in README) I get plenty of rejections. I > assume this is because of modifications made to the patches that accidentally > broke them (iiuc it's been a while since we rolled NSS). > > I can squash this change into the existing patch, but not being able to verify > that it works and to rewrite the patches that come after aesgcmchromium.patch, > I'm afraid of breaking the broken even more. > > It's perfectly possible that I missed something though and I'm trying to apply > them wrong - I will take a closer look at the rejections I get on Monday. Do you > find the bitrot hypothesis unlikely? Right, I wasn't trying to discount that somethings broke, but bitrot doesn't quite fit, since you're checking out a tagged release. That is, it's more likely that someone forgot to add a patch to applypatches.sh, at least from looking at https://chromium.googlesource.com/chromium/src/+log/master/net/third_party/nss It also appears that at least a few CLs were allowed to go by without patches (= sadness) - https://chromium.googlesource.com/chromium/src/+/0e565a979c87c9b8b25cb96d3206... - https://chromium.googlesource.com/chromium/src/+/0aad74ad1d2e14e38e5b7671de78... may have botched the patch - https://chromium.googlesource.com/chromium/src/+/3e1f77385a9feb89b0a52ecf5f6c... didn't add to applypatches I don't want you to have to go through all the hassle of rebasing, but it did seem possible to tweak into the existing patch. But perhaps I'm missing something
Patchset #3 (id:40001) has been deleted
Thanks, Ryan! In ps3 I applied the change directly in aesgcmchromium.patch . As discussed, I can't verify if it applies cleanly in the context of the entire set, because other patches in the list already don't apply. Ptal.
LGTM https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... File net/third_party/nss/patches/aesgcmchromium.patch (right): https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:12: @@ -44,6 +44,9 @@ +44,9 -> +45,9 https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:22: @@ -1842,6 +1845,69 @@ ssl3_BuildRecordPseudoHeader(unsigned ch +1845,69 -> +1846,63 (Since you deleted 6 lines, and +1 for the context offset in the new file) https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:86: @@ -1893,10 +1959,10 @@ ssl3_AESGCM(ssl3KeyMaterial *keys, +1959,10 -> +1960,10 https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:99: @@ -5103,6 +5169,10 @@ ssl3_SendClientHello(sslSocket *ss, PRBo +5169,10 -> +5170,10 https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:110: @@ -8080,6 +8150,10 @@ ssl3_HandleClientHello(sslSocket *ss, SS +8150,10 -> +8151,10
Thanks, Ryan! https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... File net/third_party/nss/patches/aesgcmchromium.patch (right): https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:12: @@ -44,6 +44,9 @@ On 2015/03/02 21:15:14, Ryan Sleevi wrote: > +44,9 -> +45,9 Done. https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:22: @@ -1842,6 +1845,69 @@ ssl3_BuildRecordPseudoHeader(unsigned ch On 2015/03/02 21:15:15, Ryan Sleevi wrote: > +1845,69 -> +1846,63 > > (Since you deleted 6 lines, and +1 for the context offset in the new file) Done. https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:86: @@ -1893,10 +1959,10 @@ ssl3_AESGCM(ssl3KeyMaterial *keys, On 2015/03/02 21:15:15, Ryan Sleevi wrote: > +1959,10 -> +1960,10 Done. https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:99: @@ -5103,6 +5169,10 @@ ssl3_SendClientHello(sslSocket *ss, PRBo On 2015/03/02 21:15:15, Ryan Sleevi wrote: > +5169,10 -> +5170,10 Done. https://codereview.chromium.org/962593002/diff/60001/net/third_party/nss/patc... net/third_party/nss/patches/aesgcmchromium.patch:110: @@ -8080,6 +8150,10 @@ ssl3_HandleClientHello(sslSocket *ss, SS On 2015/03/02 21:15:15, Ryan Sleevi wrote: > +8150,10 -> +8151,10 Done.
The CQ bit was checked by ppi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/962593002/#ps100001 (title: "Address Ryan's comments -> fix the .patch.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962593002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f7302e6c7f955f11679ab844cfe3d326f47bb0dc Cr-Commit-Position: refs/heads/master@{#318863} |
