|
|
Created:
4 years, 2 months ago by fwang Modified:
4 years, 1 month ago Reviewers:
not to use - tonikitoo, gavinp, dcheng, Jorge Lucangeli Obes, jln (very slow on Chromium), bbudge-google CC:
chromium-reviews, cbentzel+watch_chromium.org, rickyz+watch_chromium.org, gavinp+disk_chromium.org, jln+watch_chromium.org, kinuko+cache_chromium.org, tonikitoo, rjkroege, kylechar, raymes, bbudge, Roland McGrath Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace some deprecated usages of readdir_r with readdir
readdir_r is deprecated and using it with recent compilers cause some
warning that may be treated as build errors. readdir is recommended
instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html
This CL replaces some usages of readdir_r with readdir in order to fix
build errors with use_ozone=1.
R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org
BUG=457759
Committed: https://crrev.com/c2fdc2710b700158ad46111be664eac7ede0fdce
Cr-Commit-Position: refs/heads/master@{#428867}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comments about the thread-safety of readdir calls. #Patch Set 3 : Fix formatting in sandbox/linux/services/proc_util.cc. #Patch Set 4 : Keep readdir_r in proc_util.cc when OS_NACL_NONSFI is defined. #
Total comments: 2
Patch Set 5 : Address review comments by Jorge Lucangeli Obes. #
Messages
Total messages: 54 (19 generated)
Patch is untested. Note that this is also related to https://bugs.chromium.org/p/chromium/issues/detail?id=457759
My main concern is the thread-safety of readdir. The man page you linked notes that in modern implementations, multiple calls to readdir on different directory streams is safe. It appears that's true for all the readdir() changes in this patch. However, do we have some way of guaranteeing that all our readdir implementations are modern?
On 2016/10/12 21:01:55, dcheng wrote: > My main concern is the thread-safety of readdir. The man page you linked notes > that in modern implementations, multiple calls to readdir on different directory > streams is safe. It appears that's true for all the readdir() changes in this > patch. > > However, do we have some way of guaranteeing that all our readdir > implementations are modern? Just to chime in here since I filed the linked BUG. All implementations of the C library that Chromium can run with are modern. It is expected that the next POSIX spec will just require that readdir keep all its state in DIR as all implementations currently do and deprecate readdir_r. In addition, using readdir_r is a guaranteed (and probably exploitable) buffer overflow. So the choice of downsides is actually between a case that doesn't exist (readdir not being thread safe) or using an exploitable function which can't be used correctly anyway (readdir_r).
net/ lgtm
base LGTM
Description was changed from ========== Replace some deprecated usages of readdir_r with readdir readdir_r is deprecated and using it with recent compilers cause some warning that may be treated as build errors. readdir is recommended instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html This CL replaces some usages of readdir_r with readdir in order to fix build errors with use_ozone=1. R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org BUG=457759 ========== to ========== Replace some deprecated usages of readdir_r with readdir readdir_r is deprecated and using it with recent compilers cause some warning that may be treated as build errors. readdir is recommended instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html This CL replaces some usages of readdir_r with readdir in order to fix build errors with use_ozone=1. R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org BUG=457759 ==========
fwang@igalia.com changed reviewers: + jorgelo@chromium.org
Can someone please review sandbox/linux/services/proc_util.cc ?
https://codereview.chromium.org/2411833004/diff/1/sandbox/linux/services/proc... File sandbox/linux/services/proc_util.cc (right): https://codereview.chromium.org/2411833004/diff/1/sandbox/linux/services/proc... sandbox/linux/services/proc_util.cc:54: for (struct dirent* e = readdir(dir.get()); e; e = readdir(dir.get())) { Are these calls to readdir guaranteed to not race each other? What happens if more than one thread in the same process is using proc_util.cc?
https://codereview.chromium.org/2411833004/diff/1/sandbox/linux/services/proc... File sandbox/linux/services/proc_util.cc (right): https://codereview.chromium.org/2411833004/diff/1/sandbox/linux/services/proc... sandbox/linux/services/proc_util.cc:54: for (struct dirent* e = readdir(dir.get()); e; e = readdir(dir.get())) { On 2016/10/19 15:50:16, Jorge Lucangeli Obes wrote: > Are these calls to readdir guaranteed to not race each other? What happens if > more than one thread in the same process is using proc_util.cc? My understanding (and the condition on which I LGTMed the base changes) is that readdir's on "different directory streams" are safe, even if simultaneously called. Since the directory stream is scoped to just this function, in theory, it should be OK.
On 2016/10/19 22:11:39, dcheng wrote: > https://codereview.chromium.org/2411833004/diff/1/sandbox/linux/services/proc... > File sandbox/linux/services/proc_util.cc (right): > > https://codereview.chromium.org/2411833004/diff/1/sandbox/linux/services/proc... > sandbox/linux/services/proc_util.cc:54: for (struct dirent* e = > readdir(dir.get()); e; e = readdir(dir.get())) { > On 2016/10/19 15:50:16, Jorge Lucangeli Obes wrote: > > Are these calls to readdir guaranteed to not race each other? What happens if > > more than one thread in the same process is using proc_util.cc? > > My understanding (and the condition on which I LGTMed the base changes) is that > readdir's on "different directory streams" are safe, even if simultaneously > called. > > Since the directory stream is scoped to just this function, in theory, it should > be OK. That makes sense. I'd add that explanation as a comment in the code.
Thank you for the review. I uploaded a new patch to add comments in the code about the thread-safety of these readdir calls.
On 2016/10/20 01:49:31, Jorge Lucangeli Obes wrote: > That makes sense. I'd add that explanation as a comment in the code. @Jorge: review ping?
lgtm
The CQ bit was checked by fwang@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org, dcheng@chromium.org, jorgelo@chromium.org Link to the patchset: https://codereview.chromium.org/2411833004/#ps40001 (title: "Fix formatting in sandbox/linux/services/proc_util.cc.")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
So the NaCl tests are failing because readdir is not implemented: https://cs.chromium.org/chromium/src/native_client/src/nonsfi/linux/directory... I'm not sure how hard it would be to implement it in a way that satisfies the modern thread-safety requirement. However, it seems that nobody is working on Pepper/NaCl right now: https://bugs.chromium.org/p/chromium/issues/detail?id=239656#c160 Any idea?
bbudge@google.com changed reviewers: + bbudge@google.com
+mseaborn +mcgrathr for their expert opinions. -dmichael (off chromium)
On 2016/10/26 09:24:18, fwang wrote: > So the NaCl tests are failing because readdir is not implemented: > https://cs.chromium.org/chromium/src/native_client/src/nonsfi/linux/directory... > > I'm not sure how hard it would be to implement it in a way that satisfies the > modern thread-safety requirement. It should be possible to implement readdir() in nonsfi/linux/directory.c above, but it might get a bit tricky. A quicker but less elegant fix could be: * Use readdir_r() if OS_NACL_NONSFI is #defined, readdir() otherwise. * Verify that native_client/src/nonsfi/linux/directory.c checks that the filename fits into "struct dirent", to address https://crbug.com/457759. I think there's an assert() already but that might need to be made stronger.
On 2016/10/26 17:52:06, Mark Seaborn wrote: > A quicker but less elegant fix could be: > > * Use readdir_r() if OS_NACL_NONSFI is #defined, readdir() otherwise. > Yes, that's what I considered and I guess I'll do that eventually. IIUC, I only need to do that for the two places in sandbox/linux/services/proc_util.cc so that should be fine. > * Verify that native_client/src/nonsfi/linux/directory.c checks that the > filename fits into "struct dirent", to address https://crbug.com/457759. I > think there's an assert() already but that might need to be made stronger. I'll see if I can do that easily, but maybe this should be in a separate patch. My attempt for this CL was really just to fix annoying build errors for the config I use and I did not consider all the usages of readdir_r in the chromium code...
The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The bots are now green again. PTAL
On 2016/10/28 13:10:48, fwang wrote: > The bots are now green again. PTAL @Jorge Lucangeli Obes: review ping?
On 2016/10/31 14:33:30, fwang wrote: > On 2016/10/28 13:10:48, fwang wrote: > > The bots are now green again. PTAL > > @Jorge Lucangeli Obes: review ping? With regard to: """ * Verify that native_client/src/nonsfi/linux/directory.c checks that the filename fits into "struct dirent", to address https://crbug.com/457759. I think there's an assert() already but that might need to be made stronger. """ Even if you don't do it on this patch, did you at least check that the code was doing the right thing?
https://codereview.chromium.org/2411833004/diff/60001/sandbox/linux/services/... File sandbox/linux/services/proc_util.cc (right): https://codereview.chromium.org/2411833004/diff/60001/sandbox/linux/services/... sandbox/linux/services/proc_util.cc:54: #if defined(OS_NACL_NONSFI) I would add another comment here (and below under the other #if defined() explaining why this is needed. Something like: "NaCl has not implemented readdir(3)." https://codereview.chromium.org/2411833004/diff/60001/sandbox/linux/services/... sandbox/linux/services/proc_util.cc:94: struct dirent* de; This can live outside of the define, can't it?
On 2016/10/31 15:58:34, Jorge Lucangeli Obes wrote: > """ > * Verify that native_client/src/nonsfi/linux/directory.c checks that the > filename fits into "struct dirent", to address https://crbug.com/457759. I > think there's an assert() already but that might need to be made stronger. > """ > > Even if you don't do it on this patch, did you at least check that the code was > doing the right thing? Yes I did, although it's a bit hard to follow what the code will do as it depends on other linux_ calls. As I see, linux_dirent_to_nacl_dirent is called in https://cs.chromium.org/chromium/src/native_client/src/nonsfi/linux/directory... and there are two asserts to ensure the file name can be copied into the "NaCl" dirent: https://cs.chromium.org/chromium/src/native_client/src/nonsfi/linux/abi_conve...
On 2016/10/31 18:39:51, fwang wrote: > On 2016/10/31 15:58:34, Jorge Lucangeli Obes wrote: > > """ > > * Verify that native_client/src/nonsfi/linux/directory.c checks that the > > filename fits into "struct dirent", to address https://crbug.com/457759. I > > think there's an assert() already but that might need to be made stronger. > > """ > > > > Even if you don't do it on this patch, did you at least check that the code > was > > doing the right thing? > > Yes I did, although it's a bit hard to follow what the code will do as it > depends on other linux_ calls. As I see, linux_dirent_to_nacl_dirent is called > in > > https://cs.chromium.org/chromium/src/native_client/src/nonsfi/linux/directory... > > and there are two asserts to ensure the file name can be copied into the "NaCl" > dirent: > > https://cs.chromium.org/chromium/src/native_client/src/nonsfi/linux/abi_conve... Cool, thanks for checking!
On 2016/10/31 18:39:51, fwang wrote: > On 2016/10/31 15:58:34, Jorge Lucangeli Obes wrote: > > """ > > * Verify that native_client/src/nonsfi/linux/directory.c checks that the > > filename fits into "struct dirent", to address https://crbug.com/457759. I > > think there's an assert() already but that might need to be made stronger. > > """ > > > > Even if you don't do it on this patch, did you at least check that the code > was > > doing the right thing? > > Yes I did, although it's a bit hard to follow what the code will do as it > depends on other linux_ calls. As I see, linux_dirent_to_nacl_dirent is called > in > > https://cs.chromium.org/chromium/src/native_client/src/nonsfi/linux/directory... > > and there are two asserts to ensure the file name can be copied into the "NaCl" > dirent: > > https://cs.chromium.org/chromium/src/native_client/src/nonsfi/linux/abi_conve... Cool, thanks for checking!
The CQ bit was checked by fwang@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@Jorge Lucangeli Obes: Thanks for the feedback. I uploaded a new patch that addresses these comments.
lgtm
The CQ bit was unchecked by fwang@igalia.com
The CQ bit was checked by fwang@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2411833004/#ps80001 (title: "Address review comments by Jorge Lucangeli Obes.")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by fwang@igalia.com
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 ========== Replace some deprecated usages of readdir_r with readdir readdir_r is deprecated and using it with recent compilers cause some warning that may be treated as build errors. readdir is recommended instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html This CL replaces some usages of readdir_r with readdir in order to fix build errors with use_ozone=1. R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org BUG=457759 ========== to ========== Replace some deprecated usages of readdir_r with readdir readdir_r is deprecated and using it with recent compilers cause some warning that may be treated as build errors. readdir is recommended instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html This CL replaces some usages of readdir_r with readdir in order to fix build errors with use_ozone=1. R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org BUG=457759 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Replace some deprecated usages of readdir_r with readdir readdir_r is deprecated and using it with recent compilers cause some warning that may be treated as build errors. readdir is recommended instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html This CL replaces some usages of readdir_r with readdir in order to fix build errors with use_ozone=1. R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org BUG=457759 ========== to ========== Replace some deprecated usages of readdir_r with readdir readdir_r is deprecated and using it with recent compilers cause some warning that may be treated as build errors. readdir is recommended instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html This CL replaces some usages of readdir_r with readdir in order to fix build errors with use_ozone=1. R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org BUG=457759 Committed: https://crrev.com/c2fdc2710b700158ad46111be664eac7ede0fdce Cr-Commit-Position: refs/heads/master@{#428867} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c2fdc2710b700158ad46111be664eac7ede0fdce Cr-Commit-Position: refs/heads/master@{#428867}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2466773003/ by jbudorick@chromium.org. The reason for reverting is: Speculative: appears to be causing flaky crashes in android_webview_test_apk and adversely affecting the CQ as a result. e.g. https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... The last few messages in the logcat include: Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: [ERROR:simple_index_file.cc(143)] Could not get file info for /data/data/org.chromium.android_webview.shell/cache/org.chromium.android_webview/137034c0a767955d_0 Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: [ERROR:simple_index_file_posix.cc(52)] readdir /data/data/org.chromium.android_webview.shell/cache/org.chromium.android_webview: No such file or directory Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: [ERROR:simple_index_file.cc(544)] Could not reconstruct index from disk Device(06ec840ef0e949a6) 11-01 01:44:07.209 27745 27788 F chromium: [FATAL:simple_index.cc(404)] Check failed: load_result->did_load. I'll try to get a fully symbolized trace tomorrow..
Message was sent while issue was closed.
On 2016/11/01 04:20:11, jbudorick wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/2466773003/ by mailto:jbudorick@chromium.org. > > The reason for reverting is: Speculative: appears to be causing flaky crashes in > android_webview_test_apk and adversely affecting the CQ as a result. > > e.g. > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... > > The last few messages in the logcat include: > > Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: > [ERROR:simple_index_file.cc(143)] Could not get file info for > /data/data/org.chromium.android_webview.shell/cache/org.chromium.android_webview/137034c0a767955d_0 > Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: > [ERROR:simple_index_file_posix.cc(52)] readdir > /data/data/org.chromium.android_webview.shell/cache/org.chromium.android_webview: > No such file or directory > Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: > [ERROR:simple_index_file.cc(544)] Could not reconstruct index from disk > Device(06ec840ef0e949a6) 11-01 01:44:07.209 27745 27788 F chromium: > [FATAL:simple_index.cc(404)] Check failed: load_result->did_load. > > I'll try to get a fully symbolized trace tomorrow.. that trace: I 3.581s Main pid: 32343, tid: 32402, name: Chrome_IOThread >>> org.chromium.android_webview.shell <<< I 3.581s Main signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- I 3.582s Main [FATAL:simple_index.cc(404)] Check failed: load_result->did_load. I 3.582s Main I 3.582s Main Stack Trace: I 3.582s Main RELADDR FUNCTION FILE:LINE I 3.582s Main 00301e5b logging::LogMessage::~LogMessage() [redacted]/src/base/logging.cc:532 I 3.582s Main 009b552b disk_cache::SimpleIndex::MergeInitializingSet(std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >) [redacted]/src/net/disk_cache/simple/simple_index.cc:404 I 3.582s Main v------> void base::internal::FunctorTraits<void (disk_cache::SimpleIndex::*)(std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >), void>::Invoke<base::WeakPtr<disk_cache::SimpleIndex> const&, std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> > >(void (disk_cache::SimpleIndex::*)(std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >), base::WeakPtr<disk_cache::SimpleIndex> const&, std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >&&) [redacted]/src/base/bind_internal.h:214 I 3.582s Main v------> void base::internal::InvokeHelper<true, void>::MakeItSo<void (disk_cache::SimpleIndex::* const&)(std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >), base::WeakPtr<disk_cache::SimpleIndex> const&, std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> > >(void (disk_cache::SimpleIndex::* const&)(std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >), base::WeakPtr<disk_cache::SimpleIndex> const&, std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >&&) [redacted]/src/base/bind_internal.h:305 I 3.582s Main v------> void base::internal::Invoker<base::internal::BindState<void (disk_cache::SimpleIndex::*)(std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >), base::WeakPtr<disk_cache::SimpleIndex>, base::internal::PassedWrapper<std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> > > >, void ()>::RunImpl<void (disk_cache::SimpleIndex::* const&)(std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >), std::__ndk1::tuple<base::WeakPtr<disk_cache::SimpleIndex>, base::internal::PassedWrapper<std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> > > > const&, 0u, 1u>(void (disk_cache::SimpleIndex::* const&)(std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> >), std::__ndk1::tuple<base::WeakPtr<disk_cache::SimpleIndex>, base::internal::PassedWrapper<std::__ndk1::unique_ptr<disk_cache::SimpleIndexLoadResult, std::__ndk1::default_delete<disk_cache::SimpleIndexLoadResult> > > > const&, base::IndexSequence<0u, 1u>) [redacted]/src/base/bind_internal.h:364
Message was sent while issue was closed.
Thanks for the trace. Just for the record, I'm no longer working on this and I'm working this issue locally by using treat_warnings_as_errors = false |