Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
141273010:
Make CopyDirectory() not copy the read only bit on Windows. (Closed)
|
Created:
6 years, 11 months ago by M-A Ruel Modified:
6 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, csharp, Vadim Sh. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake CopyDirectory() not copy the read only bit on Windows by reimplementing it.
Add CopyDirectoryACL test similar to CopyFileACL. Now both CopyDirectory and
CopyFile exhibits the same kind of behavior.
CopyDirectory is used in unit tests, on Windows installer and on OSX
web_application.
R=thakis@chromium.org,grt@chromium.org,rvargas@chromium.org
BUG=116251
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249088
Patch Set 1 #Patch Set 2 : removed stale comment #Patch Set 3 : Made it gcc friendly by using ASSERT_FALSE() #
Total comments: 8
Patch Set 4 : Remove ifdef, use constants #
Total comments: 3
Patch Set 5 : Rewrote #Patch Set 6 : whitespace #
Total comments: 19
Patch Set 7 : address review commens #
Total comments: 2
Patch Set 8 : add brackets #
Total comments: 11
Patch Set 9 : braces #Patch Set 10 : Also fix the place I copied pasted from #Patch Set 11 : Add comment #
Total comments: 2
Patch Set 12 : Revert change to CopyFileUnsafe #
Messages
Total messages: 39 (0 generated)
Dear Nico, can you tell me why on gcc ASSERT_EQ(false, IsReadOnly(...)); doesn't compile but ASSERT_EQ(true, IsReadOnly(...)); does? And why clang doesn't have trouble with this?
On 2014/01/17 20:29:02, M-A Ruel wrote: > Dear Nico, can you tell me why on gcc ASSERT_EQ(false, IsReadOnly(...)); doesn't > compile but ASSERT_EQ(true, IsReadOnly(...)); does? And why clang doesn't have > trouble with this? Switche to ASSERT_FALSE(). I'm not sure I really want to know the number of layers of macros and templates this code goes through.
FTR, https://codereview.chromium.org/132183007 depends on this CL. Just noting because you already reviewed the other CL.
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest.... base/file_util_unittest.cc:1363: #if defined(OS_WIN) || defined(OS_POSIX) What else is there? I think you can remove this define, it's a noop, no? https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest.... base/file_util_unittest.cc:1382: EXPECT_TRUE(SetPosixFilePermissions(path, 0400)); s/0400/S_IRUSR/ https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest.... base/file_util_unittest.cc:1394: return !(mode & 0200); s/0200/S_IWUSR/ https://codereview.chromium.org/141273010/diff/70001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/70001/base/file_util_win.cc#ne... base/file_util_win.cc:95: return true; Hm, that's kinda unfortunate that this requires enumerating all copied files :-/ I don't know if the installer relies on this (add one of the installer guys), and I don't know enough windows to know if this is the right thing to do. Maybe one of the more windows base owners should look at this.
On 2014/01/21 04:12:17, Nico wrote:
> I don't know if the installer relies on this (add one of the installer guys),
> and I don't know enough windows to know if this is the right thing to do.
Maybe
> one of the more windows base owners should look at this.
+Carlos for confirmation that not copying the SECURITY_DESCRIPTOR in
CopyDirectory() is the right thing to do.
$ git grep --name-only "CopyDirectory(" | egrep -v
"_apitest|_unittest|_browsertest"
base/file_util.h
base/file_util_posix.cc
base/file_util_win.cc
chrome/browser/web_applications/web_app_mac.mm
chrome/installer/util/copy_tree_work_item.cc
chrome/installer/util/delete_tree_work_item.cc
chrome/test/automation/proxy_launcher.cc
chrome/test/perf/memory_test.cc
chrome/test/perf/perf_ui_test_suite.cc
net/disk_cache/disk_cache_test_base.cc
In particular, CopyTreeWorkItem is used in install_worker.cc via work_item.cc.
Note that the installed code has always been writeable to the account installing
it.
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest....
base/file_util_unittest.cc:1363: #if defined(OS_WIN) || defined(OS_POSIX)
On 2014/01/21 04:12:18, Nico wrote:
> What else is there? I think you can remove this define, it's a noop, no?
Oh, I was thinking about iOS but indeed, it's a no-op. Fixed here and for
CopyFileACL.
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest....
base/file_util_unittest.cc:1382: EXPECT_TRUE(SetPosixFilePermissions(path,
0400));
On 2014/01/21 04:12:18, Nico wrote:
> s/0400/S_IRUSR/
Done.
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest....
base/file_util_unittest.cc:1394: return !(mode & 0200);
On 2014/01/21 04:12:18, Nico wrote:
> s/0200/S_IWUSR/
Done.
https://codereview.chromium.org/141273010/diff/70001/base/file_util_win.cc
File base/file_util_win.cc (right):
https://codereview.chromium.org/141273010/diff/70001/base/file_util_win.cc#ne...
base/file_util_win.cc:95: return true;
On 2014/01/21 04:12:18, Nico wrote:
> Hm, that's kinda unfortunate that this requires enumerating all copied files
:-/
Indeed. It's a bit obscene in fact. Another option is to use the same code than
on POSIX.
On 2014/01/21 15:41:35, M-A Ruel wrote:
> On 2014/01/21 04:12:17, Nico wrote:
> > I don't know if the installer relies on this (add one of the installer
guys),
> > and I don't know enough windows to know if this is the right thing to do.
> Maybe
> > one of the more windows base owners should look at this.
>
> +Carlos for confirmation that not copying the SECURITY_DESCRIPTOR in
> CopyDirectory() is the right thing to do.
Redirecting to Erik and Greg since they are the ones that touched the relevant
file, chrome/installer/setup/install_worker.cc which indirectly calls
CopyDirectory()
>
> $ git grep --name-only "CopyDirectory(" | egrep -v
> "_apitest|_unittest|_browsertest"
> base/file_util.h
> base/file_util_posix.cc
> base/file_util_win.cc
> chrome/browser/web_applications/web_app_mac.mm
> chrome/installer/util/copy_tree_work_item.cc
> chrome/installer/util/delete_tree_work_item.cc
> chrome/test/automation/proxy_launcher.cc
> chrome/test/perf/memory_test.cc
> chrome/test/perf/perf_ui_test_suite.cc
> net/disk_cache/disk_cache_test_base.cc
>
> In particular, CopyTreeWorkItem is used in install_worker.cc via work_item.cc.
> Note that the installed code has always been writeable to the account
installing
> it.
>
>
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest.cc
> File base/file_util_unittest.cc (right):
>
>
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest....
> base/file_util_unittest.cc:1363: #if defined(OS_WIN) || defined(OS_POSIX)
> On 2014/01/21 04:12:18, Nico wrote:
> > What else is there? I think you can remove this define, it's a noop, no?
>
> Oh, I was thinking about iOS but indeed, it's a no-op. Fixed here and for
> CopyFileACL.
>
>
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest....
> base/file_util_unittest.cc:1382: EXPECT_TRUE(SetPosixFilePermissions(path,
> 0400));
> On 2014/01/21 04:12:18, Nico wrote:
> > s/0400/S_IRUSR/
>
> Done.
>
>
https://codereview.chromium.org/141273010/diff/70001/base/file_util_unittest....
> base/file_util_unittest.cc:1394: return !(mode & 0200);
> On 2014/01/21 04:12:18, Nico wrote:
> > s/0200/S_IWUSR/
>
> Done.
>
> https://codereview.chromium.org/141273010/diff/70001/base/file_util_win.cc
> File base/file_util_win.cc (right):
>
>
https://codereview.chromium.org/141273010/diff/70001/base/file_util_win.cc#ne...
> base/file_util_win.cc:95: return true;
> On 2014/01/21 04:12:18, Nico wrote:
> > Hm, that's kinda unfortunate that this requires enumerating all copied files
> :-/
>
> Indeed. It's a bit obscene in fact. Another option is to use the same code
than
> on POSIX.
https://codereview.chromium.org/141273010/diff/180001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/180001/base/file_util_win.cc#n... base/file_util_win.cc:76: if (SHFileOperation(&file_operation) != 0) { nit: omit braces for single-liners like this here and on lines 84, 88, and 91. https://codereview.chromium.org/141273010/diff/180001/base/file_util_win.cc#n... base/file_util_win.cc:81: FileEnumerator f(to_path, true, FileEnumerator::FILES); it looks like ShellCopy is expected to be called in some situations where |to_path| exists. this code here is going to modify every file in |to_path|, not only the ones that were copied. this doesn't seem right.
https://codereview.chromium.org/141273010/diff/180001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/180001/base/file_util_win.cc#n... base/file_util_win.cc:81: FileEnumerator f(to_path, true, FileEnumerator::FILES); On 2014/01/22 21:27:45, grt wrote: > it looks like ShellCopy is expected to be called in some situations where > |to_path| exists. this code here is going to modify every file in |to_path|, not > only the ones that were copied. this doesn't seem right. You are right. What about I rip out calls to CopyFile and SHFileOperation and use the same implementation that on posix? It would the advantage to not mess with the security descriptors or the file bits. It has the disadvantage of being potentially a tad slower but it's really only used in practice at install time to copy ~100mb of data or so, or in tests. So doing it in user mode would not be dramatic vs the way CopyFile may do it slightly more optimized. What do you think?
Since grt has looked at this I'll leave it to him.
On 2014/01/23 01:00:50, M-A Ruel wrote: > What about I rip out calls to CopyFile and SHFileOperation and use the same > implementation that on posix? It would the advantage to not mess with the > security descriptors or the file bits. It has the disadvantage of being > potentially a tad slower but it's really only used in practice at install time > to copy ~100mb of data or so, or in tests. So doing it in user mode would not be > dramatic vs the way CopyFile may do it slightly more optimized. What do you > think? This sounds okay to me. In addition to making sure that all unittests pass, please also build mini_installer.exe with a component=static_library build and confirm that the installer works and that permissions aren't somehow messed up. Also see if you can preserve the existing behavior in the face of errors. I don't know offhand if SHFileOperation undoes its work if something goes awry part-way through an operation.
On 2014/01/23 16:19:07, grt wrote:
> This sounds okay to me. In addition to making sure that all unittests pass,
> please also build mini_installer.exe with a component=static_library build and
> confirm that the installer works and that permissions aren't somehow messed
up.
Built mini_installer in Release. It immediately started up
AppData\Local\Chromium\Application\chrome.exe.
I looked up at the permissions with icacls.
. NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
BUILTIN\Administrators:(I)(OI)(CI)(F)
CONSOLE\maruel:(I)(OI)(CI)(F)
.\Application NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
BUILTIN\Administrators:(I)(OI)(CI)(F)
CONSOLE\maruel:(I)(OI)(CI)(F)
.\Application\chrome.exe NT AUTHORITY\SYSTEM:(I)(F)
BUILTIN\Administrators:(I)(F)
CONSOLE\maruel:(I)(F)
which is basically the same as in AppData\Local\Google\Chrome\User Data. Since I
have an OS-wide install, I cannot directly compare but this code is changed to
take the inheritance of the parent directory, which would be Program Files in
this case, which has a sane default SECURITY_DESCRIPTOR.
> Also see if you can preserve the existing behavior in the face of errors. I
> don't know offhand if SHFileOperation undoes its work if something goes awry
> part-way through an operation.
I failed to find a way where this is documented. I make CopyFileUnsafe() delete
partial copy but not CopyDirectory().
I also tested that this codes resolved the initial problem why this change was
done in the first place; running net_unittests from a read-only tree now works
with this patch.
https://codereview.chromium.org/141273010/diff/360001/base/file_util_posix.cc
File base/file_util_posix.cc (left):
https://codereview.chromium.org/141273010/diff/360001/base/file_util_posix.cc...
base/file_util_posix.cc:268: if (strlcpy(top_dir, from_path.value().c_str(),
There was no point in doing a strlcpy just to get the length of the string...
https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc
File base/file_util_win.cc (right):
https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n...
base/file_util_win.cc:786: std::vector<char> buffer(64*1024);
The posix version use 32kb but Windows really likes 64kb chunks.
https://codereview.chromium.org/141273010/diff/360001/base/file_util_posix.cc File base/file_util_posix.cc (right): https://codereview.chromium.org/141273010/diff/360001/base/file_util_posix.cc... base/file_util_posix.cc:266: if (strlen(from_path.value().c_str()) >= PATH_MAX) { no need for strlen, either, is there? if (from_path.value().size() >= PATH_MAX) https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:144: // This function does not properly handle destinations within the source nit: period at end of sentence https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:146: if (PathExists(real_to_path)) { do the existing unittests cover all of these cases? https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:160: real_from_path.value()) == 0) indent to align with 0 on previous line and add braces on account of multi-line-ness https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:778: kFileShareAll, kFileShareAll -> 0 https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:786: std::vector<char> buffer(64*1024); you don't seem to be using the power of vector (which, in addition to everything else, zeroes out the buffer). how about: const DWORD kBufferSize = 64 * 1024; scoped_ptr<uint8_t[]> buffer(new uint8_t[kBufferSize]); https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:787: bool result = true; fewer lines of code if result starts as false and only becomes true when all data has been copied. https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:794: if (size == 0) { omit braces on single-line conditionals
https://codereview.chromium.org/141273010/diff/360001/base/file_util_posix.cc File base/file_util_posix.cc (right): https://codereview.chromium.org/141273010/diff/360001/base/file_util_posix.cc... base/file_util_posix.cc:266: if (strlen(from_path.value().c_str()) >= PATH_MAX) { On 2014/01/24 20:50:07, grt wrote: > no need for strlen, either, is there? > if (from_path.value().size() >= PATH_MAX) Done. https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:144: // This function does not properly handle destinations within the source On 2014/01/24 20:50:07, grt wrote: > nit: period at end of sentence Done. https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:146: if (PathExists(real_to_path)) { On 2014/01/24 20:50:07, grt wrote: > do the existing unittests cover all of these cases? In FileUtilTest, - CopyDirectoryRecursivelyNew - CopyDirectoryRecursivelyExists - CopyDirectoryNew - CopyDirectoryExists - CopyFileWithCopyDirectoryRecursiveToNew - CopyFileWithCopyDirectoryRecursiveToExisting - CopyFileWithCopyDirectoryRecursiveToExistingDirectory - CopyDirectoryWithTrailingSeparators - CopyDirectoryACL does most of them. https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:160: real_from_path.value()) == 0) On 2014/01/24 20:50:07, grt wrote: > indent to align with 0 on previous line and add braces on account of > multi-line-ness Done here and in file_util_posix.cc (where I copied the code from) https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:778: kFileShareAll, On 2014/01/24 20:50:07, grt wrote: > kFileShareAll -> 0 Done. https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:786: std::vector<char> buffer(64*1024); On 2014/01/24 20:50:07, grt wrote: > you don't seem to be using the power of vector (which, in addition to everything > else, zeroes out the buffer). how about: > const DWORD kBufferSize = 64 * 1024; > scoped_ptr<uint8_t[]> buffer(new uint8_t[kBufferSize]); Done, but that still zero-out the memory. :) https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:787: bool result = true; On 2014/01/24 20:50:07, grt wrote: > fewer lines of code if result starts as false and only becomes true when all > data has been copied. Done. https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:794: if (size == 0) { On 2014/01/24 20:50:07, grt wrote: > omit braces on single-line conditionals Done.
lgtm with one final nit https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/360001/base/file_util_win.cc#n... base/file_util_win.cc:786: std::vector<char> buffer(64*1024); On 2014/01/24 21:32:57, M-A Ruel wrote: > On 2014/01/24 20:50:07, grt wrote: > > you don't seem to be using the power of vector (which, in addition to > everything > > else, zeroes out the buffer). how about: > > const DWORD kBufferSize = 64 * 1024; > > scoped_ptr<uint8_t[]> buffer(new uint8_t[kBufferSize]); > > Done, but that still zero-out the memory. :) I don't think C++03 requires this. My read of the spec is that the contents of memory returned by operator new[] are unspecified (3.7.3.1), and that for an array of a scalar type like a char, the contents of the array has indeterminate value (5.3.4 paragraph 15). https://codereview.chromium.org/141273010/diff/420001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/420001/base/file_util_win.cc#n... base/file_util_win.cc:761: to_path.value().length() >= MAX_PATH) i'd keep the braces here since the "if" statement's condition spans two lines.
Thanks! https://codereview.chromium.org/141273010/diff/420001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/420001/base/file_util_win.cc#n... base/file_util_win.cc:761: to_path.value().length() >= MAX_PATH) On 2014/01/26 23:55:08, grt wrote: > i'd keep the braces here since the "if" statement's condition spans two lines. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/141273010/500001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2014/01/27 01:00:26, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... Nico, can I have approval?
On 2014/01/27 02:16:01, M-A Ruel wrote: > Nico, can I have approval? 18 hours ping.
On 2014/01/27 20:13:58, M-A Ruel wrote: > On 2014/01/27 02:16:01, M-A Ruel wrote: > > Nico, can I have approval? > > 18 hours ping. 3:30h ping if you assume I come in at 9am.
On 2014/01/27 20:18:10, Nico wrote: > On 2014/01/27 20:13:58, M-A Ruel wrote: > > On 2014/01/27 02:16:01, M-A Ruel wrote: > > > Nico, can I have approval? > > > > 18 hours ping. > > 3:30h ping if you assume I come in at 9am. I thought you were an half-borg that never sleeps.
lgtm i guess, stampy for the windows bits based on grt's review (I think this change is pretty yucky, but I can't think of anything better.) https://codereview.chromium.org/141273010/diff/500001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/141273010/diff/500001/base/file_util_unittest... base/file_util_unittest.cc:1414: #if defined(OS_WIN) #if defined(OS_MACOSX) // OS X code #else // code for all other platforms #endif Plans to change os x? https://codereview.chromium.org/141273010/diff/500001/base/file_util_unittest... base/file_util_unittest.cc:1485: ASSERT_FALSE(IsReadOnly(dst)); same here https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc#n... base/file_util_win.cc:160: real_from_path.value()) == 0) Did this use to work? (also nit: braces are mandatory if body or condition are > 1 line)
https://codereview.chromium.org/141273010/diff/500001/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/141273010/diff/500001/base/file_util_unittest... base/file_util_unittest.cc:1414: #if defined(OS_WIN) On 2014/01/27 20:25:46, Nico wrote: > #if defined(OS_MACOSX) > // OS X code > #else > // code for all other platforms > #endif > > Plans to change os x? It's https://codereview.chromium.org/132183007 but it depends on this CL, which dragged on a little bit. Sorry for the currently confusing state of affairs. https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc#n... base/file_util_win.cc:160: real_from_path.value()) == 0) On 2014/01/27 20:25:46, Nico wrote: > Did this use to work? Yes, it was partially copy pasted from file_utils_posix.cc > (also nit: braces are mandatory if body or condition are > 1 line) I also fixed the code in where I copy-pasted from. git blames thestig and evanm in 2008~2009, which basically correspond to when I stopped doing C++. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/141273010/650001
https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc#n... base/file_util_win.cc:131: // SHFileOperation(). This used to copy the file attributes and extended While SHFileOperation is a huge function that most likely has a large amount of Crazy Code(TM), it has the advantage of... a large amount of code that handle any crazy combination of source and destination which most likely the iteration code doesn't cover "properly" :( https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc#n... base/file_util_win.cc:134: // want the containing directory to propagate its SECURITY_DESCRIPTOR. I'm not fully convinced that's what we want from a general purpose base function (although it is definitely what we want for tests). Do we want to change the owner of a file when we copy it to another location? change what other users can do with it or who can read it?. The location changed, not necessarily all attributes associated with it. Some part of me says that changing security attributes should be an explicit action (different call). On a separate note, if we go with removing ACLs, that should be documented on the header. And on a final level, the comment doesn't follow the implementation: this code relies on CopyFileUnsafe, and that code preserves security attributes and overrides the FILE_ATTRIBUTE_READONLY (which of course is not a security attribute, and seems fine to ignore)
One possible avenue is to copy the old CopyDirectory() function into the installer code base, so the installer can continue using SHFileOperation but the tests are not affected. Does this makes sense? Otherwise, I could create a CopyDirectoryWithACL() that keeps the old code. https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc#n... base/file_util_win.cc:131: // SHFileOperation(). This used to copy the file attributes and extended On 2014/01/27 20:35:33, rvargas wrote: > While SHFileOperation is a huge function that most likely has a large amount of > Crazy Code(TM), it has the advantage of... a large amount of code that handle > any crazy combination of source and destination which most likely the iteration > code doesn't cover "properly" :( That is also one of my concerns. Any idea of edge cases? https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc#n... base/file_util_win.cc:134: // want the containing directory to propagate its SECURITY_DESCRIPTOR. On 2014/01/27 20:35:33, rvargas wrote: > I'm not fully convinced that's what we want from a general purpose base function > (although it is definitely what we want for tests). Do we want to change the > owner of a file when we copy it to another location? change what other users can > do with it or who can read it?. The location changed, not necessarily all > attributes associated with it. Some part of me says that changing security > attributes should be an explicit action (different call). To be clear, this function is massively used for test, and 2 non-testing one-offs. So for example for the installer, it'd be worth doing one function for the installer that is used exclusively for it. Does that sounds like a better route? With the new code, the files inherit from the new inherited security descriptors, which is "generally" what one wants. > On a separate note, if we go with removing ACLs, that should be documented on > the header. It goes with matching with what POSIX (and soon OSX) do. > And on a final level, the comment doesn't follow the implementation: this code > relies on CopyFileUnsafe, and that code preserves security attributes and > overrides the FILE_ATTRIBUTE_READONLY (which of course is not a security > attribute, and seems fine to ignore) CopyFileUnsafe() doesn't keep FILE_ATTRIBUTE_READONLY anymore, that's the point.
To be clear, my concerns may be addressed by just fixing the comment. If we were to have two versions of CopyDirectory, it would seem better to add one version for tests and in that case call the regular function and iterate the destination removing the read only flag (basically the code of patch set 4). https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc#n... base/file_util_win.cc:131: // SHFileOperation(). This used to copy the file attributes and extended On 2014/01/27 20:42:28, M-A Ruel wrote: > On 2014/01/27 20:35:33, rvargas wrote: > > While SHFileOperation is a huge function that most likely has a large amount > of > > Crazy Code(TM), it has the advantage of... a large amount of code that handle > > any crazy combination of source and destination which most likely the > iteration > > code doesn't cover "properly" :( > > That is also one of my concerns. Any idea of edge cases? Nope. https://codereview.chromium.org/141273010/diff/500001/base/file_util_win.cc#n... base/file_util_win.cc:134: // want the containing directory to propagate its SECURITY_DESCRIPTOR. On 2014/01/27 20:42:28, M-A Ruel wrote: > On 2014/01/27 20:35:33, rvargas wrote: > > I'm not fully convinced that's what we want from a general purpose base > function > > (although it is definitely what we want for tests). Do we want to change the > > owner of a file when we copy it to another location? change what other users > can > > do with it or who can read it?. The location changed, not necessarily all > > attributes associated with it. Some part of me says that changing security > > attributes should be an explicit action (different call). > > To be clear, this function is massively used for test, and 2 non-testing > one-offs. So for example for the installer, it'd be worth doing one function for > the installer that is used exclusively for it. Does that sounds like a better > route? As is, I'm fine either way. I wish we were more explicit about base functions that are tailored for tests so that at least it is clear that using them for production code requires further inspection. grt is fine with the change, so I believe that from the point of view of the installer there is no issue using this code, whatever the name. > > With the new code, the files inherit from the new inherited security > descriptors, which is "generally" what one wants. > > > On a separate note, if we go with removing ACLs, that should be documented on > > the header. > > It goes with matching with what POSIX (and soon OSX) do. That doesn't mean it should not be documented if we set it to do the same thing on all platforms (which we are not). I don't believe our base API is meant to follow posix semantics (in general). > > > And on a final level, the comment doesn't follow the implementation: this code > > relies on CopyFileUnsafe, and that code preserves security attributes and > > overrides the FILE_ATTRIBUTE_READONLY (which of course is not a security > > attribute, and seems fine to ignore) > > CopyFileUnsafe() doesn't keep FILE_ATTRIBUTE_READONLY anymore, that's the point. Yes, that's what I said :). It removes READONLY but leaves all ACLs in place. In other words, the new code is in practice CopyDirectoryWithACL() (same as the old implementation), so the comment is not correct. I have no issue at all with removing READONLY on copy... and so I'm fine with the behavioral change. I'm a little worried by doing it with an iteration and not calling the shell, but we can probably live with that. Some weird corner case will fail, but as you said, current callers seem ok.
On 2014/01/27 22:49:17, rvargas wrote: > > > On a separate note, if we go with removing ACLs, that should be documented > on > > > the header. > > > > It goes with matching with what POSIX (and soon OSX) do. > > That doesn't mean it should not be documented if we set it to do the same thing > on all platforms (which we are not). I don't believe our base API is meant to > follow posix semantics (in general). I added a comment about OSX behaving different from other OSes in file_util.h. But right after I'll do https://codereview.chromium.org/132183007 which will standardize it for OSX too. I initially did it as two separate CLs because it was easier to test locally. > > > And on a final level, the comment doesn't follow the implementation: this > code > > > relies on CopyFileUnsafe, and that code preserves security attributes and > > > overrides the FILE_ATTRIBUTE_READONLY (which of course is not a security > > > attribute, and seems fine to ignore) > > > > CopyFileUnsafe() doesn't keep FILE_ATTRIBUTE_READONLY anymore, that's the > point. > > Yes, that's what I said :). It removes READONLY but leaves all ACLs in place. Not anymore. It now creates a fresh new file. Check patchset 11. Does patchset #11 looks good to you?
On 2014/01/28 15:23:10, M-A Ruel wrote: > On 2014/01/27 22:49:17, rvargas wrote: > > > > On a separate note, if we go with removing ACLs, that should be documented > > on > > > > the header. > > > > > > It goes with matching with what POSIX (and soon OSX) do. > > > > That doesn't mean it should not be documented if we set it to do the same > thing > > on all platforms (which we are not). I don't believe our base API is meant to > > follow posix semantics (in general). > > I added a comment about OSX behaving different from other OSes in file_util.h. > But right after I'll do https://codereview.chromium.org/132183007 which will > standardize it for OSX too. I initially did it as two separate CLs because it > was easier to test locally. > > > > > > And on a final level, the comment doesn't follow the implementation: this > > code > > > > relies on CopyFileUnsafe, and that code preserves security attributes and > > > > overrides the FILE_ATTRIBUTE_READONLY (which of course is not a security > > > > attribute, and seems fine to ignore) > > > > > > CopyFileUnsafe() doesn't keep FILE_ATTRIBUTE_READONLY anymore, that's the > > point. > > > > Yes, that's what I said :). It removes READONLY but leaves all ACLs in place. > > Not anymore. It now creates a fresh new file. Check patchset 11. Am I looking at the wrong file? I'm reading CopyFileUnsafe from rev244947. It calls ::CopyFile() and then removes FILE_ATTRIBUTE_READONLY. There's even a comment about not touching ACLs. And this code delegates the work to CopyFileUnsafe (which is good, because whatever we do, CopyFile and CopyDirectory should be consistent). So I don't think the Windows implementation after this CL would remove all metadata from the file (think any alternate data streams on the file... is that metadata?), so the comment on the header in PS 11 also seems incorrect. > > Does patchset #11 looks good to you?
On 2014/01/28 20:47:22, rvargas wrote: > On 2014/01/28 15:23:10, M-A Ruel wrote: > > On 2014/01/27 22:49:17, rvargas wrote: > > > > > On a separate note, if we go with removing ACLs, that should be > documented > > > on > > > > > the header. > > > > > > > > It goes with matching with what POSIX (and soon OSX) do. > > > > > > That doesn't mean it should not be documented if we set it to do the same > > thing > > > on all platforms (which we are not). I don't believe our base API is meant > to > > > follow posix semantics (in general). > > > > I added a comment about OSX behaving different from other OSes in file_util.h. > > But right after I'll do https://codereview.chromium.org/132183007 which will > > standardize it for OSX too. I initially did it as two separate CLs because it > > was easier to test locally. > > > > > > > > > And on a final level, the comment doesn't follow the implementation: > this > > > code > > > > > relies on CopyFileUnsafe, and that code preserves security attributes > and > > > > > overrides the FILE_ATTRIBUTE_READONLY (which of course is not a security > > > > > attribute, and seems fine to ignore) > > > > > > > > CopyFileUnsafe() doesn't keep FILE_ATTRIBUTE_READONLY anymore, that's the > > > point. > > > > > > Yes, that's what I said :). It removes READONLY but leaves all ACLs in > place. > > > > Not anymore. It now creates a fresh new file. Check patchset 11. > > Am I looking at the wrong file? I'm reading CopyFileUnsafe from rev244947. It > calls ::CopyFile() and then removes FILE_ATTRIBUTE_READONLY. There's even a > comment about not touching ACLs. And this code delegates the work to > CopyFileUnsafe (which is good, because whatever we do, CopyFile and > CopyDirectory should be consistent). > > So I don't think the Windows implementation after this CL would remove all > metadata from the file (think any alternate data streams on the file... is that > metadata?), so the comment on the header in PS 11 also seems incorrect. Look at file_util_win.cc. I change CopyFileUnsafe() to not use ::CopyFile() anymore.
https://codereview.chromium.org/141273010/diff/670001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/670001/base/file_util_win.cc#n... base/file_util_win.cc:754: // structured storage, NTFS file system alternate data streams, Having base::CopyFile() drop alternate data streams is a very surprising API IMO. While I understand that this works for tests, I don't think we should tailor the behavior of what seems to be general purpose base code to the requirements of test code. CopyFile should do what copying a file through the shell does on each platform. If POSIX doesn't support alternate data streams that behavior should not be extended to a platform that supports them just for the sake of unified behavior.
https://codereview.chromium.org/141273010/diff/670001/base/file_util_win.cc File base/file_util_win.cc (right): https://codereview.chromium.org/141273010/diff/670001/base/file_util_win.cc#n... base/file_util_win.cc:754: // structured storage, NTFS file system alternate data streams, On 2014/01/28 22:40:37, rvargas wrote: > Having base::CopyFile() drop alternate data streams is a very surprising API > IMO. > > While I understand that this works for tests, I don't think we should tailor the > behavior of what seems to be general purpose base code to the requirements of > test code. > > CopyFile should do what copying a file through the shell does on each platform. > If POSIX doesn't support alternate data streams that behavior should not be > extended to a platform that supports them just for the sake of unified behavior. Please state a single occurrence where copying the ADS or ACL is useful. I'm not only doing it for tests and consistency but also because it makes sense. Here's a few examples; - content/browser/download/save_file_manager.cc uses CopyFile() in SaveFileManager::SaveLocalFile() but it doesn't make sense to keep the ACL when "downloading" a local file. - chrome/browser/extensions/convert_user_script.cc ConvertUserScriptToExtension() uses CopyFile() to copy the file into a temp path. This means the file will keep the ACL of whatever imported extension will keep the ACL of the source file. It doesn't make sense either. - content/browser/download/base_file_posix.cc explicitly zap out file mode. - and content/browser/download/base_file_win.cc doesn't use base::CopyFile(), it calls SHFileOperation() directly. - chrome/utility/importer/ie_importer_win.cc is the only place where an ADS is actively used and *only* for reading. - cloud_print/virtual_driver/win/install/setup.cc uses CopyFile to copy its monitor dll name into a sensitive path. It doesn't make sense to keep the file "user writeable" with the potentially insecure original security descriptor. - chrome/browser/extensions/sandboxed_unpacker.cc uses CopyFile to unpack an extension into a temporary directory (which in theories will have only access by the current user). It doesn't make sense to copy a potentially wide acl in there. The exact list of files can be gathered with: git gs "CopyFile(" | egrep -v "_(unittest|browsertest|apitest)\." | grep -v "\.py"
Patchset #12 removes the change to CopyFileUnsafe(). This significantly reduce the scope of this change while keeping the end result, which is not carrying over the $%&?@! read only bit. For completeness: CopyDirectory() still has a small behavior change, it is not copying over the SECURITY_DESCRIPTOR of the directories being copied. We think in practice this is not significant in practice, since the ACL for the files is kept.
lgtm
lgtm
The CQ bit was checked by maruel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/141273010/680001
Message was sent while issue was closed.
Change committed as 249088 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
