Compiling a content extension fails when user's home directory is on a different volume from /var/tmp

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 250942 [details]
Patch v1

[Alex Christensen]

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?

[Brady Eidson]

Will be asking for the eyes of multiple reviewers.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 250944 [details]
Patch v2

[Brady Eidson]

(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)

[Oliver Hunt]

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?

[Oliver Hunt]

Comment on attachment 250944 [details]
Patch v2

R- for reasons attached to the last patch

[Brady Eidson]

(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*.

[Brady Eidson]

Created attachment 250954 [details]
Patch v3 - MMAP and tighter permissions

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 250964 [details]
Patch v4 - Fallback to NSFileManager

[WebKit Commit Bot]

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.

[Anders Carlsson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 250971 [details]
Patch v5

[Brady Eidson]

Created attachment 250973 [details]
Patch v6

[Anders Carlsson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 250977 [details]
Patch v7

[Brady Eidson]

(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.

[WebKit Commit Bot]

Comment on attachment 250977 [details]
Patch v7

Clearing flags on attachment: 250977

Committed r182932: <http://trac.webkit.org/changeset/182932>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.