Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2642483002:
Relax ScopedFD close() error checking (Closed)
DescriptionRelax ScopedFD close() error checking
This relaxes a check recently added to ScopedFDCloseTraits::Free to catch
double closes, which can be detected by checking for failures of close()
with errno of EBADF.
Unfortunately, some Linux drivers can return errors from close() other
than EBADF, but this does not indicate a failure to actually close the
file descriptor. Since r401427, such errors will cause a crash (e.g.
when unplugging your keyboard on Chrome OS - see crbug.com/681865).
Relax the check to only check for EBADF since this is the only status
that indicates a serious programming error.
BUG=660960
Review-Url: https://codereview.chromium.org/2642483002
Cr-Commit-Position: refs/heads/master@{#446119}
Committed: https://chromium.googlesource.com/chromium/src/+/b639e1d8775caaf587b58c0ff646c22a17a9aeef
Patch Set 1 #
Total comments: 2
Patch Set 2 : rm base::debug::Alias #Patch Set 3 : linux only #
Total comments: 1
Patch Set 4 : ret != 0 #Patch Set 5 : add back alias and todo #Messages
Total messages: 20 (8 generated)
spang@chromium.org changed reviewers: + davidben@chromium.org, rsesek@chromium.org, thakis@chromium.org
lgtm, though Nico and Robert should probably weigh in too. On the Linux side, here's where the kernel switched to not emit ENODEV on close, but it doesn't seem we can assume that new of a kernel. https://github.com/torvalds/linux/commit/eb38f3a4f6e86f8bb10a3217ebd85ecc5d76... And given the fix was not in telling the close implementation to ignore some class of errors, I'm sure there's other bugs of this sort lurking in there. :-/ This comment suggests the fd *is* removed from the fd table in this case, so ignoring the error won't leak: https://bugzilla.opensuse.org/show_bug.cgi?id=939571#c35 https://codereview.chromium.org/2642483002/diff/1/base/files/scoped_file.cc File base/files/scoped_file.cc (right): https://codereview.chromium.org/2642483002/diff/1/base/files/scoped_file.cc#n... base/files/scoped_file.cc:38: base::debug::Alias(&close_errno); We probably can just remove this. I believe we've since determined it was EBADF and, with the change below, we won't be aliasing any useful value anyway.
https://codereview.chromium.org/2642483002/diff/1/base/files/scoped_file.cc File base/files/scoped_file.cc (right): https://codereview.chromium.org/2642483002/diff/1/base/files/scoped_file.cc#n... base/files/scoped_file.cc:43: PCHECK(0 == ret || close_errno != EBADF); Since this is only a problem on Linux and not other POSIX, should we restrict this to just OS_LINUX?
The CQ bit was checked by spang@chromium.org to run a CQ dry run
On 2017/01/18 03:54:41, Robert Sesek wrote: > https://codereview.chromium.org/2642483002/diff/1/base/files/scoped_file.cc > File base/files/scoped_file.cc (right): > > https://codereview.chromium.org/2642483002/diff/1/base/files/scoped_file.cc#n... > base/files/scoped_file.cc:43: PCHECK(0 == ret || close_errno != EBADF); > Since this is only a problem on Linux and not other POSIX, should we restrict > this to just OS_LINUX? How does that look?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2642483002/diff/40001/base/files/scoped_file.cc File base/files/scoped_file.cc (right): https://codereview.chromium.org/2642483002/diff/40001/base/files/scoped_file.... base/files/scoped_file.cc:36: // returning some other error. Probably want to remove both the code and the TODO or neither. ;-) If we're making it OS_LINUX-only, the base::debug::Alias isn't totally useless, so I guess we can keep it around until the bugs are closed? (Though I think we know the ones we were looking at were EBADF.)
The CQ bit was checked by spang@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...
On 2017/01/18 20:38:26, davidben wrote: > https://codereview.chromium.org/2642483002/diff/40001/base/files/scoped_file.cc > File base/files/scoped_file.cc (right): > > https://codereview.chromium.org/2642483002/diff/40001/base/files/scoped_file.... > base/files/scoped_file.cc:36: // returning some other error. > Probably want to remove both the code and the TODO or neither. ;-) > > If we're making it OS_LINUX-only, the base::debug::Alias isn't totally useless, > so I guess we can keep it around until the bugs are closed? (Though I think we > know the ones we were looking at were EBADF.) Whoops about leaving the TODO. Added those bits back.
non-owner lgtm
lgtm
lgtm, though I wonder if a whitelist of allowed error codes might be better (e.g. only ignore ENODEV -- EINTR is already handled by IGNORE_EINTR) (+jln, author of that "It's important to close" comment in https://codereview.chromium.org/183953004 -- but since ENODEV doesn't point to an actual close failure, I think that part is fine.)
On 2017/01/25 16:34:45, Nico wrote: > lgtm, though I wonder if a whitelist of allowed error codes might be better > (e.g. only ignore ENODEV -- EINTR is already handled by IGNORE_EINTR) > There aren't any error codes on Linux that can cause close to fail in the way described by the comment. Structurally, the first thing the close syscall does is remove the file descriptor table entry; the only precondition for this part is that the file descriptor exists. Of course the kernel may change but as there is a strong desire in the kernel community for close never to fail, and because in the current implementation of the syscall there is no code path that fails to close, it seems very unlikely that to me that this check will ever fire for a useful purpose on Linux. > (+jln, author of that "It's important to close" comment in > https://codereview.chromium.org/183953004 -- but since ENODEV doesn't point to > an actual close failure, I think that part is fine.)
The CQ bit was checked by spang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485370183369770,
"parent_rev": "b461901d4922144f8cd88810a60e23a3a5d08e4c", "commit_rev":
"b639e1d8775caaf587b58c0ff646c22a17a9aeef"}
Message was sent while issue was closed.
Description was changed from ========== Relax ScopedFD close() error checking This relaxes a check recently added to ScopedFDCloseTraits::Free to catch double closes, which can be detected by checking for failures of close() with errno of EBADF. Unfortunately, some Linux drivers can return errors from close() other than EBADF, but this does not indicate a failure to actually close the file descriptor. Since r401427, such errors will cause a crash (e.g. when unplugging your keyboard on Chrome OS - see crbug.com/681865). Relax the check to only check for EBADF since this is the only status that indicates a serious programming error. BUG=660960 ========== to ========== Relax ScopedFD close() error checking This relaxes a check recently added to ScopedFDCloseTraits::Free to catch double closes, which can be detected by checking for failures of close() with errno of EBADF. Unfortunately, some Linux drivers can return errors from close() other than EBADF, but this does not indicate a failure to actually close the file descriptor. Since r401427, such errors will cause a crash (e.g. when unplugging your keyboard on Chrome OS - see crbug.com/681865). Relax the check to only check for EBADF since this is the only status that indicates a serious programming error. BUG=660960 Review-Url: https://codereview.chromium.org/2642483002 Cr-Commit-Position: refs/heads/master@{#446119} Committed: https://chromium.googlesource.com/chromium/src/+/b639e1d8775caaf587b58c0ff646... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/b639e1d8775caaf587b58c0ff646... |
