Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1491743009:
[base] POSIX File::Unlock() didn't actually unlock file. (Closed)
|
Created:
5 years ago by Scott Hess - ex-Googler Modified:
5 years ago Reviewers:
Lei Zhang CC:
chromium-reviews, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[base] POSIX File::Unlock() didn't actually unlock file.
Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in
flock.type. As a result, the file is not actually unlocked.
F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on
the file descriptor, which is why the call succeeds. The only flag
defined appears to be FD_CLOEXEC, which is the low-order bit of the
third parameter. Since the parameter passed to fcntl is aligned at
off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can
tell cases with FD_CLOEXEC do not use base::File.
[Addendum: File::Lock/Unlock was landed for leveldb, and at this time
Chromium's only use is in third_party/leveldatabase/env_chromium.cc.]
BUG=565787
Committed: https://crrev.com/e66da3f22aee5e74d107f961843c07b8d4e311f3
Cr-Commit-Position: refs/heads/master@{#364497}
Patch Set 1 #Patch Set 2 : Try without fix to see what happens. #Patch Set 3 : Back to original CL. #Patch Set 4 : forgot some comments #
Total comments: 4
Patch Set 5 : Move lock tests to new file, with test fixture, share code. #
Total comments: 6
Messages
Total messages: 22 (6 generated)
Try without fix to see what happens.
Back to original CL.
forgot some comments
shess@chromium.org changed reviewers: + thestig@chromium.org
Previous fcntl CL was because I was checking the locking strategy for purposes of prototyping code to depend on it. This CL is because such code doesn't work because the existing file locking is broken on POSIX. The testing is ... I don't know what to say about it. It seems excessive, but using fcntl locks on POSIX means that you cannot test things in-process. Spinning up a separate process for testing means you need a way to talk to it for purposes of verifying that you're testing the right things and not receiving false positives. PS1 is running a try to see what this breaks. PS2 is running a try to see what the tests show when the fix isn't present. Looks like iOS doesn't allow spawning processes, so that will have to be accommodated.
How did we survive for this? Is there no bug #? How did you even find this problem? https://codereview.chromium.org/1491743009/diff/60001/base/files/file_unittes... File base/files/file_unittest.cc (right): https://codereview.chromium.org/1491743009/diff/60001/base/files/file_unittes... base/files/file_unittest.cc:617: base::Process test_child_process = Check |test_child_process| is valid before continuing? Ditto below. https://codereview.chromium.org/1491743009/diff/60001/base/files/file_unittes... base/files/file_unittest.cc:666: CHECK(WaitForEvent(temp_path, kSignalLockFileClose)); Would it easier to have one child process impl that has a switch and can respond to any arbitrary set of commands?
On 2015/12/04 03:23:03, Lei Zhang wrote: > How did you even find this problem? Oh, I should have read your comments more carefully, duh.
On 2015/12/04 03:23:03, Lei Zhang wrote: > How did we survive for this? Is there no bug #? How did you even find this > problem? I was definitely "o_O" when I realized that my unlocks weren't working. AFAICT, it works because nobody ever uses it. I don't know if that's because it didn't work, so when people used it it was flaky, or if all of the use cases closed or exited and released the lock that way, or what. The only place which immediately sprang to mind was the process singleton, but it uses atomic file operations so as to work across network mounts, and anyhow if that weren't working we'd definitely have noticed. Brr. I renamed Lock/Unlock to xLock/xUnlock, and the first compile error I get is in third_party/leveldatabase/env_chromium.cc. Kinda terrified right now. I _think_ since it never releases the lock, it shouldn't lead anywhere, but IMHO that's a delicate user. Maybe I'll go create that bug to refer to. https://codereview.chromium.org/1491743009/diff/60001/base/files/file_unittes... File base/files/file_unittest.cc (right): https://codereview.chromium.org/1491743009/diff/60001/base/files/file_unittes... base/files/file_unittest.cc:666: CHECK(WaitForEvent(temp_path, kSignalLockFileClose)); On 2015/12/04 03:23:03, Lei Zhang wrote: > Would it easier to have one child process impl that has a switch and can respond > to any arbitrary set of commands? Just to clarify, you mean to merge the different child processes and have the switch for unlock, unlock-by-close, and unlock-by-exit? That would make sense. I'm not entirely sure that the unlock-by-close and unlock-by-exit cases are worth testing, but once one is tested the others aren't hard to add.
Description was changed from ========== [base] POSIX File::Unlock() didn't actually unlock file. Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in flock.type. As a result, the file is not actually unlocked. F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on the file descriptor, which is why the call succeeds. The only flag defined appears to be FD_CLOEXEC, which is the low-order bit of the third parameter. Since the parameter passed to fcntl is aligned at off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can tell cases with FD_CLOEXEC do not use base::File. BUG=none ========== to ========== [base] POSIX File::Unlock() didn't actually unlock file. Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in flock.type. As a result, the file is not actually unlocked. F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on the file descriptor, which is why the call succeeds. The only flag defined appears to be FD_CLOEXEC, which is the low-order bit of the third parameter. Since the parameter passed to fcntl is aligned at off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can tell cases with FD_CLOEXEC do not use base::File. BUG=565787 ==========
https://codereview.chromium.org/1491743009/diff/60001/base/files/file_unittes... File base/files/file_unittest.cc (right): https://codereview.chromium.org/1491743009/diff/60001/base/files/file_unittes... base/files/file_unittest.cc:666: CHECK(WaitForEvent(temp_path, kSignalLockFileClose)); On 2015/12/04 06:40:34, Scott Hess wrote: > On 2015/12/04 03:23:03, Lei Zhang wrote: > > Would it easier to have one child process impl that has a switch and can > respond > > to any arbitrary set of commands? > > Just to clarify, you mean to merge the different child processes and have the > switch for unlock, unlock-by-close, and unlock-by-exit? That would make sense. If it's easy to implement and it reduces the amount of code, then sure. You'll need to be able to wait for multiple events. If that turns out to be too complicated, then let's just stick with the current code, but consolidate the common code at the beginning into a ChildSetup() function. > I'm not entirely sure that the unlock-by-close and unlock-by-exit cases are > worth testing, but once one is tested the others aren't hard to add. Might as well.
Move lock tests to new file, with test fixture, share code.
On 2015/12/08 00:46:20, Scott Hess wrote: > Move lock tests to new file, with test fixture, share code. Sorry it doesn't diff well against the previous CL. Oh, well. It probably wouldn't diff well in place, either. Justification for moving to a new test file is: - iOS doesn't support sub-processes, so otherwise it would be an #ifdef. - Sharing the code without a test fixture needed like four pointers into the calling context. I dislike the "overloading" of SignalEvent() and WaitForEventOrTimeout(), but I also disliked tarting them up with faux namespacing. So I throw myself on your mercy.
https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... File base/files/file_locking_unittest.cc (right): https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... base/files/file_locking_unittest.cc:135: ASSERT_EQ(File::FILE_OK, lock_file_.Unlock()); I just did a test reverting the file_posix.cc change, and the two lines above should be removed. With these lines, the bug is noticed in the CHECK_EQ() where the child attempts the lock, because the parent didn't actually release it, and it's noticed in every test. Without these lines, the bug is noticed in the LockAndUnlock test only, at the point where kSignalLockFileUnlocked was signalled and the parent couldn't get the lock. That seems much better to me.
Description was changed from ========== [base] POSIX File::Unlock() didn't actually unlock file. Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in flock.type. As a result, the file is not actually unlocked. F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on the file descriptor, which is why the call succeeds. The only flag defined appears to be FD_CLOEXEC, which is the low-order bit of the third parameter. Since the parameter passed to fcntl is aligned at off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can tell cases with FD_CLOEXEC do not use base::File. BUG=565787 ========== to ========== [base] POSIX File::Unlock() didn't actually unlock file. Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in flock.type. As a result, the file is not actually unlocked. F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on the file descriptor, which is why the call succeeds. The only flag defined appears to be FD_CLOEXEC, which is the low-order bit of the third parameter. Since the parameter passed to fcntl is aligned at off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can tell cases with FD_CLOEXEC do not use base::File. [Addendum: File::Lock/Unlock was landed for leveldb, and at this time Chromium's only use is in third_party/leveldatabase/env_chromium.cc.] BUG=565787 ==========
lgtm https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... File base/files/file_locking_unittest.cc (right): https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... base/files/file_locking_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. no: (c) https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... base/files/file_locking_unittest.cc:44: File::FLAG_CREATE | File::FLAG_APPEND); Do you need FLAG_APPEND? https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... base/files/file_locking_unittest.cc:135: ASSERT_EQ(File::FILE_OK, lock_file_.Unlock()); On 2015/12/08 00:55:43, Scott Hess wrote: > I just did a test reverting the file_posix.cc change, and the two lines above > should be removed. With these lines, the bug is noticed in the CHECK_EQ() where > the child attempts the lock, because the parent didn't actually release it, and > it's noticed in every test. Without these lines, the bug is noticed in the > LockAndUnlock test only, at the point where kSignalLockFileUnlocked was > signalled and the parent couldn't get the lock. That seems much better to me. Acknowledged.
Thanks! https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... File base/files/file_locking_unittest.cc (right): https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... base/files/file_locking_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/12/10 04:02:52, Lei Zhang wrote: > no: (c) FML. https://codereview.chromium.org/1491743009/diff/80001/base/files/file_locking... base/files/file_locking_unittest.cc:44: File::FLAG_CREATE | File::FLAG_APPEND); On 2015/12/10 04:02:52, Lei Zhang wrote: > Do you need FLAG_APPEND? I cannot say why I used that (I'd guess I was figuring out the set of O_ flags at some point before refactoring). Changed to FLAG_WRITE in opposition to FLAG_READ below. Since there's no actual communication in the file itself, mostly I'm just aiming for a pair of realistic open calls which won't tickle some "Not allowed" edge case.
The CQ bit was checked by shess@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491743009/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491743009/80001
Message was sent while issue was closed.
Description was changed from ========== [base] POSIX File::Unlock() didn't actually unlock file. Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in flock.type. As a result, the file is not actually unlocked. F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on the file descriptor, which is why the call succeeds. The only flag defined appears to be FD_CLOEXEC, which is the low-order bit of the third parameter. Since the parameter passed to fcntl is aligned at off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can tell cases with FD_CLOEXEC do not use base::File. [Addendum: File::Lock/Unlock was landed for leveldb, and at this time Chromium's only use is in third_party/leveldatabase/env_chromium.cc.] BUG=565787 ========== to ========== [base] POSIX File::Unlock() didn't actually unlock file. Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in flock.type. As a result, the file is not actually unlocked. F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on the file descriptor, which is why the call succeeds. The only flag defined appears to be FD_CLOEXEC, which is the low-order bit of the third parameter. Since the parameter passed to fcntl is aligned at off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can tell cases with FD_CLOEXEC do not use base::File. [Addendum: File::Lock/Unlock was landed for leveldb, and at this time Chromium's only use is in third_party/leveldatabase/env_chromium.cc.] BUG=565787 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [base] POSIX File::Unlock() didn't actually unlock file. Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in flock.type. As a result, the file is not actually unlocked. F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on the file descriptor, which is why the call succeeds. The only flag defined appears to be FD_CLOEXEC, which is the low-order bit of the third parameter. Since the parameter passed to fcntl is aligned at off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can tell cases with FD_CLOEXEC do not use base::File. [Addendum: File::Lock/Unlock was landed for leveldb, and at this time Chromium's only use is in third_party/leveldatabase/env_chromium.cc.] BUG=565787 ========== to ========== [base] POSIX File::Unlock() didn't actually unlock file. Unlock passed F_UNLCK as cmd to fcntl(). It's supposed to be in flock.type. As a result, the file is not actually unlocked. F_UNLCK is 2 (on OSX and Linux), cmd 2 is F_SETFD, which sets flags on the file descriptor, which is why the call succeeds. The only flag defined appears to be FD_CLOEXEC, which is the low-order bit of the third parameter. Since the parameter passed to fcntl is aligned at off_t or better, Unlock() will clear FD_CLOEXEC if set. As best I can tell cases with FD_CLOEXEC do not use base::File. [Addendum: File::Lock/Unlock was landed for leveldb, and at this time Chromium's only use is in third_party/leveldatabase/env_chromium.cc.] BUG=565787 Committed: https://crrev.com/e66da3f22aee5e74d107f961843c07b8d4e311f3 Cr-Commit-Position: refs/heads/master@{#364497} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e66da3f22aee5e74d107f961843c07b8d4e311f3 Cr-Commit-Position: refs/heads/master@{#364497} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
