Bug 168241

Summary:

Disable mmap'd cache files if container is class A

Product:

WebKit

Reporter:

JF Bastien <jfbastien>

Component:

WebKit2

Assignee:

JF Bastien <jfbastien>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, cdumez, cgarcia, commit-queue, fpizlo, ggaren, jfbastien, koivisto, oliver, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	koivisto: review+
patch	cdumez: commit-queue-
patch	none
patch	none

JF Bastien

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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

JF Bastien

Comment 1 2017-02-13 11:35:04 PST

<rdar://problem/23676252>

JF Bastien

Comment 2 2017-02-13 11:39:26 PST

Created attachment 301365 [details] patch Attaching patch by Antti.

Antti Koivisto

Comment 3 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.

Chris Dumez

Comment 4 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.

Chris Dumez

Comment 5 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.

Antti Koivisto

Comment 6 2017-02-13 12:23:59 PST

Created attachment 301372 [details] patch

Chris Dumez

Comment 7 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 ?

Chris Dumez

Comment 8 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.

Antti Koivisto

Comment 9 2017-02-13 12:32:20 PST

Created attachment 301375 [details] patch

Chris Dumez

Comment 10 2017-02-13 12:33:42 PST

Comment on attachment 301375 [details] patch lgtm

Antti Koivisto

Comment 11 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.

Chris Dumez

Comment 12 2017-02-13 13:05:57 PST

Comment on attachment 301381 [details] patch lgtm.

JF Bastien

Comment 13 2017-02-13 13:10:36 PST

lgtm here too, I've been trying out the patch locally and it looks stable.

Antti Koivisto

Comment 14 2017-02-13 13:32:45 PST

https://trac.webkit.org/changeset/212244

Note You need to log in before you can comment on or make changes to this bug.