Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2678813003:
Disable fd/handle sharing in base::ReadFileToString. (Closed)
|
Created:
3 years, 10 months ago by grt (UTC plus 2) Modified:
3 years, 10 months ago Reviewers:
gab CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable fd/handle sharing in base::ReadFileToString.
This CL adds the macro "FONE" to disable fd/handle sharing when opening
files via base::OpenFile/fopen, and adds one use of it in
ReadFileToString where it seems clear that such sharing is never
desirable.
BUG=671990, 688362
R=gab@chromium.org
Patch Set 1 #
Total comments: 10
Patch Set 2 : comment tweaks #
Messages
Total messages: 17 (9 generated)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
PTAL
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: This issue passed the CQ dry run.
Meta-comment: This default behavior is surprising to me. Would be great to require an explicit call to get this behavior, e.g. OpenInheritableFile(). Perhaps we can add that call (implemented as a mere redirect to OpenFile() for now), ping the owners of the few callers that look like they could benefit from inheritance (shared memory? tracing? maybe not even if our sandbox blocks handle inheritance?) and migrate as needed. Then flip the switch to block this by default in OpenFile()? https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.cc File base/files/file_util.cc (right): https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.cc#new... base/files/file_util.cc:159: CloseFile(file); Since we CloseFile here, is the issue that |file| can be inherited by child processes created by another thread while it happens to be open? i.e. this is racy? Sounds like such inheritance is pretty much never desired..? https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h#newc... base/files/file_util.h:43: // across platforms. Define the macro FONE (i.e., "fopen 'N' or 'e'") for use in Why "FONE"? https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h#newc... base/files/file_util.h:49: #else // !defined(OS_WIN) Just use // defined(OS_WIN) everywhere (i.e. here and on #endif), that's more common than toggling IMO (as toggling becomes complicated in complex #ifs)
The CQ bit was checked by grt@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...
Since this is causing problems on the bots, WDYT of landing this for now while having a broader discussion among the base OWNERs of what the API should really look like? https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.cc File base/files/file_util.cc (right): https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.cc#new... base/files/file_util.cc:159: CloseFile(file); On 2017/02/06 21:13:19, gab wrote: > Since we CloseFile here, is the issue that |file| can be inherited by child > processes created by another thread while it happens to be open? i.e. this is > racy? Yes, racy and actively happening in tests due to the aggressive parallelism of the test launcher. > Sounds like such inheritance is pretty much never desired..? That's my thinking, but it is the default on POSIX, so likely the default on Windows as well. I think it makes sense to flip Chromium's default around (see internal thread), but this incremental change here is at least in the right direction. https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h#newc... base/files/file_util.h:43: // across platforms. Define the macro FONE (i.e., "fopen 'N' or 'e'") for use in On 2017/02/06 21:13:19, gab wrote: > Why "FONE"? FO -> fopen N -> 'N' on Windows E -> 'e' on glibc platforms I was trying to convey that in the comment with "fopen 'N' or 'e'". This was somewhat inspired by base/format_macros.h, which uses the prefix PRI for macros that pertain to printf. https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h#newc... base/files/file_util.h:49: #else // !defined(OS_WIN) On 2017/02/06 21:13:19, gab wrote: > Just use // defined(OS_WIN) everywhere (i.e. here and on #endif), that's more > common than toggling IMO (as toggling becomes complicated in complex #ifs) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/06 21:51:21, grt (UTC plus 1) wrote: > Since this is causing problems on the bots, WDYT of landing this for now while > having a broader discussion among the base OWNERs of what the API should really > look like? How badly is it failing though? Hasn't this been the case forever? Won't the problem subsist in tests in smaller pockets of the codebase until it's applied to all? I'm afraid a quick fix will result in never proceeding with updating this to the desired API. https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h#newc... base/files/file_util.h:43: // across platforms. Define the macro FONE (i.e., "fopen 'N' or 'e'") for use in On 2017/02/06 21:51:21, grt (UTC plus 1) wrote: > On 2017/02/06 21:13:19, gab wrote: > > Why "FONE"? > > FO -> fopen > N -> 'N' on Windows > E -> 'e' on glibc platforms > > I was trying to convey that in the comment with "fopen 'N' or 'e'". This was > somewhat inspired by base/format_macros.h, which uses the prefix PRI for macros > that pertain to printf. That's very obscure to me. How about NOINHERITANCE or something explicit? https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h#newc... base/files/file_util.h:328: // (defined above) when composing a |mode| string to disable this. Mention that this is prefered for new callers and add TODO to flip default
On 2017/02/07 19:42:59, gab wrote: > On 2017/02/06 21:51:21, grt (UTC plus 1) wrote: > > Since this is causing problems on the bots, WDYT of landing this for now while > > having a broader discussion among the base OWNERs of what the API should > really > > look like? > > How badly is it failing though? To my knowledge, all tests that use the test launcher (nearly all, I think) leak .tmp files, leading to wasted disk space on the bots. And then there's this https://groups.google.com/a/chromium.org/d/msg/chromium-dev/69ITDy_itZo/mR-J5..., which I fully agree with. In this case, it's not a test that's leaving behind files, but the test infrastructure itself. I closed this gap as much as I could in r447996, but now the problem is in ReadFileToString. > Hasn't this been the case forever? It's been the case since the test launcher gained its current parallel approach. I'm not sure how long that's been. > Won't the > problem subsist in tests in smaller pockets of the codebase until it's applied > to all? No. The specific problem leading to leaks in tests is addressed by this change. > I'm afraid a quick fix will result in never proceeding with updating > this to the desired API. I feel you. I don't particularly want to take on bringing us to the desired API today -- my cup runneth over, as they say (I'd rather get back to the whole MoveFile debacle!). That said, I don't see the harm in this change -- it's very low impact (fixes a bug) and opens the door to other spot fixes.
https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h#newc... base/files/file_util.h:43: // across platforms. Define the macro FONE (i.e., "fopen 'N' or 'e'") for use in On 2017/02/07 19:42:59, gab wrote: > On 2017/02/06 21:51:21, grt (UTC plus 1) wrote: > > On 2017/02/06 21:13:19, gab wrote: > > > Why "FONE"? > > > > FO -> fopen > > N -> 'N' on Windows > > E -> 'e' on glibc platforms > > > > I was trying to convey that in the comment with "fopen 'N' or 'e'". This was > > somewhat inspired by base/format_macros.h, which uses the prefix PRI for > macros > > that pertain to printf. > > That's very obscure to me. How does PRIuS sit with you (https://cs.chromium.org/search/?q=PRIuS+file:src/base/format_macros%5C.h&type... > How about NOINHERITANCE or something explicit? I could do that, but it's quite long. :-)
On 2017/02/07 20:35:55, grt (UTC plus 1) wrote: > On 2017/02/07 19:42:59, gab wrote: > > On 2017/02/06 21:51:21, grt (UTC plus 1) wrote: > > > Since this is causing problems on the bots, WDYT of landing this for now > while > > > having a broader discussion among the base OWNERs of what the API should > > really > > > look like? > > > > How badly is it failing though? > > To my knowledge, all tests that use the test launcher (nearly all, I think) leak > .tmp files, leading to wasted disk space on the bots. And then there's this > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/69ITDy_itZo/mR-J5..., > which I fully agree with. In this case, it's not a test that's leaving behind > files, but the test infrastructure itself. I closed this gap as much as I could > in r447996, but now the problem is in ReadFileToString. > > > Hasn't this been the case forever? > > It's been the case since the test launcher gained its current parallel approach. > I'm not sure how long that's been. This has been >3 years IMO... > > > Won't the > > problem subsist in tests in smaller pockets of the codebase until it's applied > > to all? > > No. The specific problem leading to leaks in tests is addressed by this change. Ok. > > > I'm afraid a quick fix will result in never proceeding with updating > > this to the desired API. > > I feel you. I don't particularly want to take on bringing us to the desired API > today -- my cup runneth over, as they say (I'd rather get back to the whole > MoveFile debacle!). That said, I don't see the harm in this change -- it's very > low impact (fixes a bug) and opens the door to other spot fixes. Well it fixes a visible bug but leaves behind a bunch of invisible ones and that's what I'm afraid of.. Can we at least update the API documentation to mention that this is prefered with a TODO to flip the default? https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2678813003/diff/1/base/files/file_util.h#newc... base/files/file_util.h:43: // across platforms. Define the macro FONE (i.e., "fopen 'N' or 'e'") for use in On 2017/02/07 20:36:06, grt (UTC plus 1) wrote: > On 2017/02/07 19:42:59, gab wrote: > > On 2017/02/06 21:51:21, grt (UTC plus 1) wrote: > > > On 2017/02/06 21:13:19, gab wrote: > > > > Why "FONE"? > > > > > > FO -> fopen > > > N -> 'N' on Windows > > > E -> 'e' on glibc platforms > > > > > > I was trying to convey that in the comment with "fopen 'N' or 'e'". This was > > > somewhat inspired by base/format_macros.h, which uses the prefix PRI for > > macros > > > that pertain to printf. > > > > That's very obscure to me. > > How does PRIuS sit with you > (https://cs.chromium.org/search/?q=PRIuS+file:src/base/format_macros%5C.h&type... I don't like it either :) > > > How about NOINHERITANCE or something explicit? > > I could do that, but it's quite long. :-) I don't think long matters, readable does. And if it becomes annoying that's one more reason to flip the default ;)
Closing this issue in favor of https://codereview.chromium.org/2687713003/.
Description was changed from ========== Disable fd/handle sharing in base::ReadFileToString. This CL adds the macro "FONE" to disable fd/handle sharing when opening files via base::OpenFile/fopen, and adds one use of it in ReadFileToString where it seems clear that such sharing is never desirable. BUG=671990,688362 R=gab@chromium.org ========== to ========== Disable fd/handle sharing in base::ReadFileToString. This CL adds the macro "FONE" to disable fd/handle sharing when opening files via base::OpenFile/fopen, and adds one use of it in ReadFileToString where it seems clear that such sharing is never desirable. BUG=671990,688362 R=gab@chromium.org ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
