Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2404823002:
Fix error handling in POSIX version of the base::File::GetLength. (Closed)
|
Created:
4 years, 2 months ago by Michael K. (Yandex Team) Modified:
4 years ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix error handling in POSIX version of the base::File::GetLength.
According to the base/files/file.h, the GetLength() method should return
a negative number on failure.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/089ec1a2cbc7c118531871fece5ba2f1328e23ac
Cr-Commit-Position: refs/heads/master@{#437512}
Patch Set 1 #Patch Set 2 : Fix error handling in POSIX version of the base::File::GetLength. #
Total comments: 3
Patch Set 3 : Fix error handling in POSIX version of the base::File::GetLength. #Patch Set 4 : Fix error handling in POSIX version of the base::File::GetLength. #Patch Set 5 : Fix error handling in POSIX version of the base::File::GetLength. #Messages
Total messages: 61 (25 generated)
Description was changed from ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. ========== to ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. ==========
mkolom@yandex-team.ru changed reviewers: + danakj@chromium.org
PTAL
I wanted to look at callsites to make sure they're doing it right, but -1 looks like it could break a lot of callers, wow. Maybe it would be ok because we return -1 on windows.. but maybe not and we should fix these. Can you have a look at them too? Can you fix this to check <0 not ==-1? https://cs.chromium.org/chromium/src/base/files/memory_mapped_file_posix.cc?r... Can you make this check the return value? https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_brows... And here? https://cs.chromium.org/chromium/src/components/nacl/renderer/plugin/pnacl_co... This should be ASSERT_GT? https://cs.chromium.org/chromium/src/components/search_provider_logos/logo_ca... This needs to handle negatives? https://cs.chromium.org/chromium/src/components/subresource_filter/content/br... This should handle negative? https://cs.chromium.org/chromium/src/components/subresource_filter/core/brows... VerifyHeader needs to handle negative? https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi... This should handle negative? https://cs.chromium.org/chromium/src/net/disk_cache/simple/simple_test_util.c... And this should? https://cs.chromium.org/chromium/src/net/disk_cache/simple/simple_test_util.c... As should this? https://cs.chromium.org/chromium/src/net/disk_cache/simple/simple_test_util.c...
Description was changed from ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. ========== to ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Sorry for the delay. PTAL. On 2016/10/10 20:21:31, danakj wrote: > And here? > https://cs.chromium.org/chromium/src/components/nacl/renderer/plugin/pnacl_co... It seems there is no error there: that file size is used in ReportTranslationFinished(..) for reporting histograms only. > This needs to handle negatives? > https://cs.chromium.org/chromium/src/components/subresource_filter/content/br... This file doesn't exist anymore. And I've found other places: sandbox/win/src/process_mitigations_test.cc net/disk_cache/blockfile/file_ios.cc net/disk_cache/blockfile/file_posix.cc net/disk_cache/blockfile/mapped_file.cc net/disk_cache/blockfile/mapped_file_avoid_mmap_posix.cc net/disk_cache/blockfile/mapped_file_posix.cc (net/disk_cache/blockfile/file.h has base::File inside and defines its own Set/GetLength based on it. Plz take a look into net/disk_cache/blockfile/file_win.cc first to understand why I made these fixes for net/disk_cache/blockfile/* files).
Thanks :) https://codereview.chromium.org/2404823002/diff/20001/net/disk_cache/blockfil... File net/disk_cache/blockfile/mapped_file.cc (right): https://codereview.chromium.org/2404823002/diff/20001/net/disk_cache/blockfil... net/disk_cache/blockfile/mapped_file.cc:26: if (!file_len) I don't think this is strictly needed. Reading 0 is okay, and would have returned true before? We shouldn't change any behaviour in this CL. https://codereview.chromium.org/2404823002/diff/20001/net/disk_cache/blockfil... File net/disk_cache/blockfile/mapped_file_avoid_mmap_posix.cc (right): https://codereview.chromium.org/2404823002/diff/20001/net/disk_cache/blockfil... net/disk_cache/blockfile/mapped_file_avoid_mmap_posix.cc:22: return NULL; 0 should be okay again for this CL https://codereview.chromium.org/2404823002/diff/20001/net/disk_cache/blockfil... File net/disk_cache/blockfile/mapped_file_posix.cc (right): https://codereview.chromium.org/2404823002/diff/20001/net/disk_cache/blockfil... net/disk_cache/blockfile/mapped_file_posix.cc:25: return NULL; same same
Ok, I'll fix it. But just to be sure you've got it right: that GetLength() method returns zero in case of an error or when a file has zero size. And mmapping zero size area isn't allowed in POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html (But in real life it depends on OS or its version) Thus to eliminate such differences I decided to make these changes. And we need to handle GetLength() errors of course too.
On Mon, Nov 28, 2016 at 11:38 PM, <mkolom@yandex-team.ru> wrote: > Ok, I'll fix it. > > But just to be sure you've got it right: that GetLength() method returns > zero in > case of an error or when a file has zero size. > And mmapping zero size area isn't allowed in POSIX: > http://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html > (But in real life it depends on OS or its version) > Thus to eliminate such differences I decided to make these changes. > And we need to handle GetLength() errors of course too. > I'm okay with the idea of reading 0 taking a different path. But for this CL we're changing error to return -1 instead of 0 and my goal here is to maintain the same behaviour in callers before and after this CL to minimize risk. Changing behaviour for 0-reads can be done in smaller CLs afterward if u want. Do you agree what I'm asking will preserve the same behaviour for all code after this CL as it was before? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/30 00:05:38, danakj wrote: > Do you agree what I'm asking will preserve the same behaviour > for all code after this CL as it was before? Yes, I agree. PTAL
There's no diff from patchset 2, looks like something went wrong with ur upload
On 2016/12/01 00:56:28, danakj wrote: > There's no diff from patchset 2, looks like something went wrong with ur upload These files were in patchset 2 but they are not in patchset 3: net/disk_cache/blockfile/mapped_file.cc net/disk_cache/blockfile/mapped_file_avoid_mmap_posix.cc net/disk_cache/blockfile/mapped_file_posix.cc
Oh sorry, got it. LGTM
The CQ bit was checked by mkolom@yandex-team.ru
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by mkolom@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2404823002/#ps60001 (title: "Fix error handling in POSIX version of the base::File::GetLength.")
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...)
I'm sorry: simple_test_util.cc was incompilable on non-MSVC :( PTAL.
LGTM. You'll need to grab owners for places outside of base/ though.
The CQ bit was checked by mkolom@yandex-team.ru
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 ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
PTAL
LGTM on components/subresource_filter/.
LGTM components/search_provider_logos/
LGTM for net/
LGTM for renderer_host/media/audio_input_debug_writer_unittest.cc
lgtm for webrtc code
mkolom@yandex-team.ru changed reviewers: + forshaw@chromium.org - cpu@chromium.org
mkolom@yandex-team.ru changed reviewers: + jschuh@chromium.org - forshaw@chromium.org
jschuh, PTAL.
On 2016/12/08 09:52:13, mkolom wrote: > jschuh, PTAL. sandbox lgtm
The CQ bit was checked by mkolom@yandex-team.ru
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by mkolom@yandex-team.ru
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by mkolom@yandex-team.ru
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
Failed to apply patch for
components/subresource_filter/core/browser/ruleset_service.cc:
While running git apply --index -p1;
error: patch failed:
components/subresource_filter/core/browser/ruleset_service.cc:346
error: components/subresource_filter/core/browser/ruleset_service.cc: patch
does not apply
Patch: components/subresource_filter/core/browser/ruleset_service.cc
Index: components/subresource_filter/core/browser/ruleset_service.cc
diff --git a/components/subresource_filter/core/browser/ruleset_service.cc
b/components/subresource_filter/core/browser/ruleset_service.cc
index
3a0c5fcb3a4b2992e3bf3e8efc2b06a4231e9f00..6c80bb00e6b429e5cf1ed56112748e087793d2c8
100644
--- a/components/subresource_filter/core/browser/ruleset_service.cc
+++ b/components/subresource_filter/core/browser/ruleset_service.cc
@@ -346,6 +346,8 @@ bool RulesetService::IndexRuleset(base::File
unindexed_ruleset_file,
SCOPED_UMA_HISTOGRAM_TIMER("SubresourceFilter.IndexRuleset.WallDuration");
int64_t unindexed_ruleset_size = unindexed_ruleset_file.GetLength();
+ if (unindexed_ruleset_size < 0)
+ return false;
CopyingFileInputStream copying_stream(std::move(unindexed_ruleset_file));
google::protobuf::io::CopyingInputStreamAdaptor zero_copy_stream_adaptor(
©ing_stream, 4096 /* buffer_size */);
The CQ bit was checked by mkolom@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, jschuh@chromium.org, chcunningham@chromium.org, phoglund@chromium.org, engedy@chromium.org, justincohen@chromium.org, agl@chromium.org Link to the patchset: https://codereview.chromium.org/2404823002/#ps80001 (title: "Fix error handling in POSIX version of the base::File::GetLength.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rebased due to the conflict.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481275080981900,
"parent_rev": "a6e159ebb48efbf82ddc26f85b7bacb88934be01", "commit_rev":
"a97a7e30746655316b02d39b6a855faedf9f9591"}
Message was sent while issue was closed.
Description was changed from ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2404823002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2404823002 ========== to ========== Fix error handling in POSIX version of the base::File::GetLength. According to the base/files/file.h, the GetLength() method should return a negative number on failure. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/089ec1a2cbc7c118531871fece5ba2f1328e23ac Cr-Commit-Position: refs/heads/master@{#437512} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/089ec1a2cbc7c118531871fece5ba2f1328e23ac Cr-Commit-Position: refs/heads/master@{#437512} |
