Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
639253004:
Allow POSIX callers to specify a new file's mode. (Closed)
DescriptionAllow POSIX callers to specify a new file's mode.
Retain the tight 0640 mode for most existing callers, but honor the user's
umask for downloaded files.
BUG=409416, 362887
R=asanka@chromium.org, mdempsky@chromium.org, rvargas@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/3457f738b0dc2dd7d67fa84148d72066db9e0519
Patch Set 1 #
Total comments: 2
Patch Set 2 : Return bool instead of int, to accord with in-progress refactoring. #
Total comments: 8
Patch Set 3 : Use ScopedFD, rename function, adhere to the bool return value. #
Total comments: 4
Patch Set 4 : Fix nits. #
Total comments: 2
Patch Set 5 : Don't mess with the umask. No need, and it causes problems. #Patch Set 6 : Correct behavior. #
Total comments: 7
Patch Set 7 : Set the right mode in case the new_path already existed. #
Messages
Total messages: 36 (9 generated)
palmer@chromium.org changed reviewers: + mdempsky@chromium.org, rvargas@chromium.org
mdempsky: Can you please give this a quick sanity check before I ping rvargas?
On 2014/10/16 22:51:49, Chromium Palmer wrote: > mdempsky: Can you please give this a quick sanity check before I ping rvargas? lgtm
Thanks, mdempsky. rvargas, can you please give this your OWNERS eye? Thanks!
looks good https://codereview.chromium.org/639253004/diff/1/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/639253004/diff/1/base/files/file_util.h#newco... base/files/file_util.h:338: BASE_EXPORT int WriteFileMode(const FilePath& filename, const char* data, This function should return bool. See crbug.com/418837
https://codereview.chromium.org/639253004/diff/1/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/639253004/diff/1/base/files/file_util.h#newco... base/files/file_util.h:338: BASE_EXPORT int WriteFileMode(const FilePath& filename, const char* data, On 2014/10/17 22:28:58, rvargas wrote: > This function should return bool. See crbug.com/418837 Done.
https://codereview.chromium.org/639253004/diff/20001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/639253004/diff/20001/base/files/file_util.h#n... base/files/file_util.h:338: BASE_EXPORT bool WriteFileMode(const FilePath& filename, const char* data, Shouldn't this be called WriteFileWithMode? WriteFileMode sounds like it's setting the mode to do a file write instead of writing. https://codereview.chromium.org/639253004/diff/20001/base/files/file_util_pos... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/639253004/diff/20001/base/files/file_util_pos... base/files/file_util_posix.cc:701: return -1; return false https://codereview.chromium.org/639253004/diff/20001/base/files/file_util_pos... base/files/file_util_posix.cc:704: if (IGNORE_EINTR(close(fd)) < 0) nit: Now that we are writing this function from scratch, we should use ScopedFD instead. https://codereview.chromium.org/639253004/diff/20001/content/browser/download... File content/browser/download/base_file_posix.cc (right): https://codereview.chromium.org/639253004/diff/20001/content/browser/download... content/browser/download/base_file_posix.cc:22: if (base::WriteFileMode(new_path, "", 0, mode) != 0) The signature changed
https://codereview.chromium.org/639253004/diff/20001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/639253004/diff/20001/base/files/file_util.h#n... base/files/file_util.h:338: BASE_EXPORT bool WriteFileMode(const FilePath& filename, const char* data, On 2014/10/21 01:05:53, rvargas wrote: > Shouldn't this be called WriteFileWithMode? WriteFileMode sounds like it's > setting the mode to do a file write instead of writing. Done. https://codereview.chromium.org/639253004/diff/20001/base/files/file_util_pos... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/639253004/diff/20001/base/files/file_util_pos... base/files/file_util_posix.cc:701: return -1; On 2014/10/21 01:05:53, rvargas wrote: > return false Done. https://codereview.chromium.org/639253004/diff/20001/base/files/file_util_pos... base/files/file_util_posix.cc:704: if (IGNORE_EINTR(close(fd)) < 0) On 2014/10/21 01:05:53, rvargas wrote: > nit: Now that we are writing this function from scratch, we should use ScopedFD > instead. Done. https://codereview.chromium.org/639253004/diff/20001/content/browser/download... File content/browser/download/base_file_posix.cc (right): https://codereview.chromium.org/639253004/diff/20001/content/browser/download... content/browser/download/base_file_posix.cc:22: if (base::WriteFileMode(new_path, "", 0, mode) != 0) On 2014/10/21 01:05:53, rvargas wrote: > The signature changed Done.
lgtm https://codereview.chromium.org/639253004/diff/40001/base/files/file_util_pos... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/639253004/diff/40001/base/files/file_util_pos... base/files/file_util_posix.cc:699: int fd = HANDLE_EINTR(creat(filename.value().c_str(), mode)); tiny nit: declare fd (or file) directly to be a ScopedFD here, and use is_valid() below. https://codereview.chromium.org/639253004/diff/40001/base/files/file_util_pos... base/files/file_util_posix.cc:704: bool status = WriteFileDescriptor(file.get(), data, size); tiny nit: return directly here.
https://codereview.chromium.org/639253004/diff/40001/base/files/file_util_pos... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/639253004/diff/40001/base/files/file_util_pos... base/files/file_util_posix.cc:699: int fd = HANDLE_EINTR(creat(filename.value().c_str(), mode)); On 2014/10/22 00:57:38, rvargas wrote: > tiny nit: declare fd (or file) directly to be a ScopedFD here, and use > is_valid() below. Done. https://codereview.chromium.org/639253004/diff/40001/base/files/file_util_pos... base/files/file_util_posix.cc:704: bool status = WriteFileDescriptor(file.get(), data, size); On 2014/10/22 00:57:38, rvargas wrote: > tiny nit: return directly here. Done.
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639253004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
palmer@chromium.org changed reviewers: + asanka@chromium.org
Oops, forgot to add asanka for content/browser/download/OWNERS approval.
palmer@chromium.org changed reviewers: + rdsmith@chromium.org
+rdsmith for OWNERS in case asanka is too busy
https://codereview.chromium.org/639253004/diff/60001/content/browser/download... File content/browser/download/base_file_posix.cc (right): https://codereview.chromium.org/639253004/diff/60001/content/browser/download... content/browser/download/base_file_posix.cc:17: mode_t mask = umask(0); Ruh roh. I think we should avoid a umask(0) call since our worker pool etc. might be busy creating files elsewhere. AIUI, creat() will honor the umask. So a stat() following a WriteFileWithMode(...0666) should have sufficed?
On 2014/10/23 06:36:16, asanka wrote: > https://codereview.chromium.org/639253004/diff/60001/content/browser/download... > File content/browser/download/base_file_posix.cc (right): > > https://codereview.chromium.org/639253004/diff/60001/content/browser/download... > content/browser/download/base_file_posix.cc:17: mode_t mask = umask(0); > Ruh roh. I think we should avoid a umask(0) call since our worker pool etc. > might be busy creating files elsewhere. > > AIUI, creat() will honor the umask. So a stat() following a > WriteFileWithMode(...0666) should have sufficed? 0644, I mean.
https://codereview.chromium.org/639253004/diff/60001/content/browser/download... File content/browser/download/base_file_posix.cc (right): https://codereview.chromium.org/639253004/diff/60001/content/browser/download... content/browser/download/base_file_posix.cc:17: mode_t mask = umask(0); > Ruh roh. I think we should avoid a umask(0) call since our worker pool etc. > might be busy creating files elsewhere. > > AIUI, creat() will honor the umask. So a stat() following a > WriteFileWithMode(...0666) should have sufficed? Oh yeah, you're right. Sorry about that. The bummer is there is no way to simply *get* the umask; you have to change it and then change it back. Between those 2 actions, other threads might have done various things. So, I don't think we can effectively stat-and-check without creating the same race condition. Also, you are right that the umask is applied as part of creat(2), so unless the kernel misbehaves, we got what we want.
https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... File content/browser/download/base_file_posix.cc (right): https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... content/browser/download/base_file_posix.cc:20: if (!base::WriteFileWithMode(new_path, "", 0, 0666) || I misspoke in my earlier comment regarding using 0666 as the initial mode. What about 0644 to be conservative? I believe that's what it used to be before the change to base::WriteFile(). https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... content/browser/download/base_file_posix.cc:24: mode = status.st_mode & 0777; If a file exists at |new_path| we should use the mode from that file rather than using 0600. That way we won't change the mode if the user chooses to overwrite an existing file.
https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... File content/browser/download/base_file_posix.cc (right): https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... content/browser/download/base_file_posix.cc:20: if (!base::WriteFileWithMode(new_path, "", 0, 0666) || > I misspoke in my earlier comment regarding using 0666 as the initial mode. What > about 0644 to be conservative? I believe that's what it used to be before the > change to base::WriteFile(). The bug this CL fixes is to respect the user's umask for downloads. 0644 does not do that. We now have WriteFile and WriteFileMode; WriteFile remains conservative, using mode 0640. https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... content/browser/download/base_file_posix.cc:24: mode = status.st_mode & 0777; > If a file exists at |new_path| we should use the mode from that file rather than > using 0600. That way we won't change the mode if the user chooses to overwrite > an existing file. I think that would surprise people.
Whoops. Sorry about the delay. The code lgtm modulo a favorable resolution to the question about file modes when a download overwrites an existing file. https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... File content/browser/download/base_file_posix.cc (right): https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... content/browser/download/base_file_posix.cc:20: if (!base::WriteFileWithMode(new_path, "", 0, 0666) || On 2014/10/24 18:35:35, Chromium Palmer wrote: > > I misspoke in my earlier comment regarding using 0666 as the initial mode. > What > > about 0644 to be conservative? I believe that's what it used to be before the > > change to base::WriteFile(). > > The bug this CL fixes is to respect the user's umask for downloads. 0644 does > not do that. > > We now have WriteFile and WriteFileMode; WriteFile remains conservative, using > mode 0640. Acknowledged. https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... content/browser/download/base_file_posix.cc:24: mode = status.st_mode & 0777; On 2014/10/24 18:35:35, Chromium Palmer wrote: > > If a file exists at |new_path| we should use the mode from that file rather > than > > using 0600. That way we won't change the mode if the user chooses to overwrite > > an existing file. > > I think that would surprise people. Would it? Let's say someone is downloading foo.txt to their downloads directory and overwriting each time. The first time it would get a mode of 0666 & ~umask (say it's 0640). The second time around, the file mode would be 0600.
https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... File content/browser/download/base_file_posix.cc (right): https://codereview.chromium.org/639253004/diff/100001/content/browser/downloa... content/browser/download/base_file_posix.cc:24: mode = status.st_mode & 0777; > Would it? Let's say someone is downloading foo.txt to their downloads directory > and overwriting each time. The first time it would get a mode of 0666 & ~umask > (say it's 0640). The second time around, the file mode would be 0600. Ahh, yes. I see what you mean now. You are right. New patchset coming up.
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639253004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639253004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3457f738b0dc2dd7d67fa84148d72066db9e0519 Cr-Commit-Position: refs/heads/master@{#301932}
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 3457f738b0dc2dd7d67fa84148d72066db9e0519 (presubmit successful).
Message was sent while issue was closed.
Reverted in e0c8785ea110e0a91523caf09100745924670693. Breaks FileUtilTest.WriteFileWithMode in Android (dbg). [ RUN ] FileUtilTest.WriteFileWithMode ../../base/files/file_util_unittest.cc:2164: Failure Value of: status.st_mode & 0777 Actual: 384 Expected: mode Which is: 420 [ FAILED ] FileUtilTest.WriteFileWithMode (1 ms) https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
