Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104533002:
Ensure native library isn't actually loaded, and no IO on net thread (Closed)
|
Created:
4 years, 5 months ago by pauljensen Modified:
4 years, 5 months ago Reviewers:
xunjieli CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure native library isn't actually loaded, and no IO on net thread
LoadNativeLibrary asserts because IO is possible, so call dlopen()
directly with flag ensuring no IO actually happens. We need to
access libnetd_client.so, which is guaranteed to be loaded by the
point where it's needed as socket() is guaranteed to be called
which libnetd_client.so handles.
BUG=623555
R=xunjieli
Committed: https://crrev.com/25ea5bc3b93a92ced13ac18936a34053521c4bca
Cr-Commit-Position: refs/heads/master@{#402817}
Patch Set 1 #Patch Set 2 : bug link #
Total comments: 2
Patch Set 3 : rebase #Patch Set 4 : redo with dlopen directly #
Total comments: 4
Patch Set 5 : address comments #Messages
Total messages: 26 (9 generated)
Helen, PTAL.
https://codereview.chromium.org/2104533002/diff/20001/net/udp/udp_socket_posi... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2104533002/diff/20001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:355: base::GetNativeLibraryName(base::ASCIIToUTF16("netd_client")))); If we already have netd_client loaded in Chromium's address space, why do we need to load it here? In other words, when will setNetworkForSocket == nullptr?
https://codereview.chromium.org/2104533002/diff/20001/net/udp/udp_socket_posi... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2104533002/diff/20001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:355: base::GetNativeLibraryName(base::ASCIIToUTF16("netd_client")))); On 2016/06/27 18:54:16, xunjieli wrote: > If we already have netd_client loaded in Chromium's address space, why do we > need to load it here? In other words, when will setNetworkForSocket == nullptr? setNetworkForSocket is nullptr when the executable starts executing. It's set by line 361 below. libnetd_client.so will be loaded indirectly by the executable calling socket() in UDPSocketPosix::Open(). See how socket() in libc.so calls into libnetd_client.so: http://androidxref.com/6.0.1_r10/xref/bionic/libc/bionic/socket.cpp#22 http://androidxref.com/6.0.1_r10/xref/bionic/libc/bionic/NetdClient.cpp#37 I just need to get the base::NativeLibrary pointer here so I can pass it to base::GetFunctionPointerFromNativeLibrary(). Normally dlopen() goes and loads the library, but then the library is already loaded it just gives back the handle to it. dlopen() man page says "If the same library is loaded again with dlopen(), the same file handle is returned." I've verified this by reading Android's dlopen code and also I wrote an example and strace'd it to verify, see my notes in the linked bug.
lgtm. Thanks for the explanations! I was reading the code wrong. It makes more sense to me now.
The CQ bit was checked by pauljensen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2104533002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Ignore spurious IO assertion failure when loading already loaded lib When LoadNativeLibrary is used to load libnetd_client.so, ignore IO assertion check in LoadNativeLibrary as libnetd_client.so is already guaranteed to be loaded at this point so no disk access is needed. socket() is guaranteed to be called prior to this point and socket() is handled by libnetd_client.so. I've verified with strace that no IO is performed. BUG=623555 R=xunjieli ========== to ========== Ensure native library isn't actually loaded, and no IO on net thread LoadNativeLibrary asserts because IO is possible, so call dlopen() directly with flag ensuring no IO actually happens. We need to access libnetd_client.so, which is guaranteed to be loaded by the point where it's needed as socket() is guaranteed to be called which libnetd_client.so handles. BUG=623555 R=xunjieli ==========
Helen, so it appears the ScopedAllowIO thing is quite prohibited (I can't upload without --bypass-hooks and it fails presubmit bot). I've reworked the CL to instead call dlopen() directly with a flag that ensures no IO is done. I need to do some testing of this change, but can you start reviewing it?
On 2016/06/28 13:00:19, pauljensen wrote: > Helen, so it appears the ScopedAllowIO thing is quite prohibited (I can't upload > without --bypass-hooks and it fails presubmit bot). I've reworked the CL to > instead call dlopen() directly with a flag that ensures no IO is done. I need > to do some testing of this change, but can you start reviewing it? This seems to work fine, PTAL.
https://codereview.chromium.org/2104533002/diff/60001/net/udp/udp_socket_posi... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2104533002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:7: #include <dlfcn.h> Should this header be in the #if defined(OS_ANDROID) ? https://codereview.chromium.org/2104533002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:359: void* dl = dlopen(file.value().c_str(), RTLD_NOW | RTLD_NOLOAD); Could add a brief comment on the flags (RTLD_NOW | RTLD_NOLOAD)? I checked the documentation, but am not quite sure why these two are chosen.
https://codereview.chromium.org/2104533002/diff/60001/net/udp/udp_socket_posi... File net/udp/udp_socket_posix.cc (right): https://codereview.chromium.org/2104533002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:7: #include <dlfcn.h> On 2016/06/29 13:35:09, xunjieli wrote: > Should this header be in the #if defined(OS_ANDROID) ? Done. https://codereview.chromium.org/2104533002/diff/60001/net/udp/udp_socket_posi... net/udp/udp_socket_posix.cc:359: void* dl = dlopen(file.value().c_str(), RTLD_NOW | RTLD_NOLOAD); On 2016/06/29 13:35:09, xunjieli wrote: > Could add a brief comment on the flags (RTLD_NOW | RTLD_NOLOAD)? I checked the > documentation, but am not quite sure why these two are chosen. Done.
lgtm
The CQ bit was checked by pauljensen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Ensure native library isn't actually loaded, and no IO on net thread LoadNativeLibrary asserts because IO is possible, so call dlopen() directly with flag ensuring no IO actually happens. We need to access libnetd_client.so, which is guaranteed to be loaded by the point where it's needed as socket() is guaranteed to be called which libnetd_client.so handles. BUG=623555 R=xunjieli ========== to ========== Ensure native library isn't actually loaded, and no IO on net thread LoadNativeLibrary asserts because IO is possible, so call dlopen() directly with flag ensuring no IO actually happens. We need to access libnetd_client.so, which is guaranteed to be loaded by the point where it's needed as socket() is guaranteed to be called which libnetd_client.so handles. BUG=623555 R=xunjieli ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Ensure native library isn't actually loaded, and no IO on net thread LoadNativeLibrary asserts because IO is possible, so call dlopen() directly with flag ensuring no IO actually happens. We need to access libnetd_client.so, which is guaranteed to be loaded by the point where it's needed as socket() is guaranteed to be called which libnetd_client.so handles. BUG=623555 R=xunjieli ========== to ========== Ensure native library isn't actually loaded, and no IO on net thread LoadNativeLibrary asserts because IO is possible, so call dlopen() directly with flag ensuring no IO actually happens. We need to access libnetd_client.so, which is guaranteed to be loaded by the point where it's needed as socket() is guaranteed to be called which libnetd_client.so handles. BUG=623555 R=xunjieli Committed: https://crrev.com/25ea5bc3b93a92ced13ac18936a34053521c4bca Cr-Commit-Position: refs/heads/master@{#402817} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/25ea5bc3b93a92ced13ac18936a34053521c4bca Cr-Commit-Position: refs/heads/master@{#402817} |
