Bug 168241 - Disable mmap'd cache files if container is class A
Summary: Disable mmap'd cache files if container is class A
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-13 11:33 PST by JF Bastien
Modified: 2017-02-13 13:32 PST (History)
10 users (show)

See Also:


Attachments
patch (7.03 KB, patch)
2017-02-13 11:39 PST, JF Bastien
koivisto: review+
Details | Formatted Diff | Diff
patch (6.77 KB, patch)
2017-02-13 12:23 PST, Antti Koivisto
cdumez: commit-queue-
Details | Formatted Diff | Diff
patch (6.90 KB, patch)
2017-02-13 12:32 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (8.42 KB, patch)
2017-02-13 13:02 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2017-02-13 11:33:41 PST
If the mmap'd cache file gets evicted and the device is locked, then WebContent won't be able to bring the file back in under class A, causing SIGBUS.
Comment 1 JF Bastien 2017-02-13 11:35:04 PST
<rdar://problem/23676252>
Comment 2 JF Bastien 2017-02-13 11:39:26 PST
Created attachment 301365 [details]
patch

Attaching patch by Antti.
Comment 3 Antti Koivisto 2017-02-13 11:50:53 PST
Comment on attachment 301365 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301365&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:149
> +    os_log(OS_LOG_DEFAULT, "protectionClass=%d", attrBuffer.protectionClass);

I'll upload a version without the stray os_log.
Comment 4 Chris Dumez 2017-02-13 11:55:08 PST
Comment on attachment 301365 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301365&action=review

r=me

> Source/WebKit2/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=168241

Please include radar url too.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:134
> +bool isSafeToUseSharedMemory(const String& path)

I would have called this simply "canUseSharedMemoryForPath()".

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:149
> +    os_log(OS_LOG_DEFAULT, "protectionClass=%d", attrBuffer.protectionClass);

I don't think we should call os_log() directly. We have the RELEASE_LOG macro for this.
Comment 5 Chris Dumez 2017-02-13 11:55:58 PST
(In reply to comment #3)
> Comment on attachment 301365 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301365&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:149
> > +    os_log(OS_LOG_DEFAULT, "protectionClass=%d", attrBuffer.protectionClass);
> 
> I'll upload a version without the stray os_log.

You cannot review your own patch :P r=me though.
Comment 6 Antti Koivisto 2017-02-13 12:23:59 PST
Created attachment 301372 [details]
patch
Comment 7 Chris Dumez 2017-02-13 12:25:07 PST
Comment on attachment 301372 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301372&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:156
> +        RELEASE_LOG(Network, "Disallowing use of shared mapped memory due to container protection class %d", attrBuffer.protectionClass);

%u ?
Comment 8 Chris Dumez 2017-02-13 12:27:07 PST
Comment on attachment 301372 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301372&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:148
> +    if (error)

We may want to RELEASE_LOG_ERROR() in this case too.
Comment 9 Antti Koivisto 2017-02-13 12:32:20 PST
Created attachment 301375 [details]
patch
Comment 10 Chris Dumez 2017-02-13 12:33:42 PST
Comment on attachment 301375 [details]
patch

lgtm
Comment 11 Antti Koivisto 2017-02-13 13:02:55 PST
Created attachment 301381 [details]
patch

Cache::store needs canUseSharedMemoryForBodyData() test too for the case where we switch an existing resource to use mapped file backing after it has been written to disk.
Comment 12 Chris Dumez 2017-02-13 13:05:57 PST
Comment on attachment 301381 [details]
patch

lgtm.
Comment 13 JF Bastien 2017-02-13 13:10:36 PST
lgtm here too, I've been trying out the patch locally and it looks stable.
Comment 14 Antti Koivisto 2017-02-13 13:32:45 PST
https://trac.webkit.org/changeset/212244