Compiling a content extension fails when user's home directory is on a different volume from /var/tmp When compiling the extension to a temporary file, the extension is then moved to ~/Library via "rename", but rename() fails when moving across volumes. In the case where rename() fails due to a cross-device error, fallback to the standard "copy file contents then delete the old file" mechanism.
Created attachment 250942 [details] Patch v1
Comment on attachment 250942 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=250942&action=review > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:84 > + // Open the file for writing only, creating it, and failing if it already exists. > + int outModeFlags = O_WRONLY | O_CREAT | O_EXCL; Why do we want to fail if it already exists? Shouldn't moveFile overwrite any existing file?
Will be asking for the eyes of multiple reviewers.
(In reply to comment #2) > Comment on attachment 250942 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250942&action=review > > > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:84 > > + // Open the file for writing only, creating it, and failing if it already exists. > > + int outModeFlags = O_WRONLY | O_CREAT | O_EXCL; > > Why do we want to fail if it already exists? Shouldn't moveFile overwrite > any existing file? In general we're quite conservative in our FS operations in WebCore/Kit. But I suppose the semantics of rename() are that the old file at the target location is first unlinked. In that this supports rename(), I'll make that change.
Created attachment 250944 [details] Patch v2
(Yes, I understand that the semantics of `mv` are also to overwrite the target file by default, but moveFile itself is not being exposed to clients. If it were I'd likely include an "overwrite?" option that defaults to false)
Comment on attachment 250942 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=250942&action=review You should probably just mmap the source file and then blat it down in a single write. Code should end up simpler as well. I'm also concerned about permission elevation as mentioned inline. > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:89 > + outFD = open(newPath.data(), outModeFlags, outPermissionFlags); Why are we making the extension world readable?
Comment on attachment 250944 [details] Patch v2 R- for reasons attached to the last patch
(In reply to comment #7) > Comment on attachment 250942 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250942&action=review > > You should probably just mmap the source file and then blat it down in a > single write. Code should end up simpler as well. I'm also concerned about > permission elevation as mentioned inline. > That's a good idea! > > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:89 > > + outFD = open(newPath.data(), outModeFlags, outPermissionFlags); > > Why are we making the extension world readable? I was wearing two hats - Fixing content extensions, and writing a more general purpose function. I agree, no need for world (or even group!) readability *right now*.
Created attachment 250954 [details] Patch v3 - MMAP and tighter permissions
Comment on attachment 250954 [details] Patch v3 - MMAP and tighter permissions NM Anders wants us to not provide a POSIX impl and use NSFileManager instead.
Created attachment 250964 [details] Patch v4 - Fallback to NSFileManager
Attachment 250964 [details] did not pass style-queue: ERROR: Source/WebCore/platform/FileSystem.cpp:102: Tab found; better to use spaces [whitespace/tab] [1] ERROR: Source/WebCore/platform/FileSystem.cpp:103: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 250964 [details] Patch v4 - Fallback to NSFileManager View in context: https://bugs.webkit.org/attachment.cgi?id=250964&action=review > Source/WebCore/platform/mac/FileSystemMac.mm:73 > + NSError *error = nil; > + [[NSFileManager defaultManager] moveItemAtPath:(NSString *)oldPath toPath:(NSString *)newPath error:&error]; The right way to test for success is to check the return value, not whether error is nil or not (that's not guaranteed). This should use the moveItemAtURL:toURL:error: method (Together with -[NSURL fileURLWithPath:]. > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:94 > + int result = rename(oldPathFsRep.data(), newPathFsRep.data()); > + if (!result) > + return true; > + > + if (errno != EXDEV) > + return false; > + > + // The paths exist on different devices so a simple rename will not work. > + // We have to manually move the file. > + return moveFile(oldPath, newPath); > } Instead of modifying the behavior of renameFile, I think you should just make whoever calls renameFile call moveFile instead.
(In reply to comment #14) > Comment on attachment 250964 [details] > Patch v4 - Fallback to NSFileManager > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250964&action=review > > > Source/WebCore/platform/mac/FileSystemMac.mm:73 > > + NSError *error = nil; > > + [[NSFileManager defaultManager] moveItemAtPath:(NSString *)oldPath toPath:(NSString *)newPath error:&error]; > > The right way to test for success is to check the return value, not whether > error is nil or not (that's not guaranteed). Fair enough. > > This should use the moveItemAtURL:toURL:error: method (Together with -[NSURL > fileURLWithPath:]. Since we already have fully qualified paths, it seems wasteful to use the URL form unless it actually has advantages other than "urls seem nicer". > > > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:94 > > + int result = rename(oldPathFsRep.data(), newPathFsRep.data()); > > + if (!result) > > + return true; > > + > > + if (errno != EXDEV) > > + return false; > > + > > + // The paths exist on different devices so a simple rename will not work. > > + // We have to manually move the file. > > + return moveFile(oldPath, newPath); > > } > > Instead of modifying the behavior of renameFile, I think you should just > make whoever calls renameFile call moveFile instead. There's only one caller. Maybe renameFile should just be the NSFileManager version on Mac.
Created attachment 250971 [details] Patch v5
Created attachment 250973 [details] Patch v6
Comment on attachment 250973 [details] Patch v6 Sorry for being unclear. Here's what I think we should do: - renameFile should keep its semantics of being an atomic POSIX rename. - The WebKit2 call to renameFile should be a call to moveFile instead.
(In reply to comment #18) > Comment on attachment 250973 [details] > Patch v6 > > Sorry for being unclear. Here's what I think we should do: > > - renameFile should keep its semantics of being an atomic POSIX rename. > - The WebKit2 call to renameFile should be a call to moveFile instead. If we do that, renameFile will be dead code. Single call site in WK2 is the only caller. I'll nuke it and make WK2 use moveFile instead.
Created attachment 250977 [details] Patch v7
(In reply to comment #19) > (In reply to comment #18) > If we do that, renameFile will be dead code. Single call site in WK2 is the > only caller. In addition, that call site only compiles on Mac/iOS right now, so we only need the NSFileManager version of the call.
Comment on attachment 250977 [details] Patch v7 Clearing flags on attachment: 250977 Committed r182932: <http://trac.webkit.org/changeset/182932>
All reviewed patches have been landed. Closing bug.