Bug 168241 – Disable mmap'd cache files if container is class A

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[JF Bastien]

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.

[JF Bastien]

<rdar://problem/23676252>

[JF Bastien]

Created attachment 301365 [details]
patch

Attaching patch by Antti.

[Antti Koivisto]

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 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]

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

Created attachment 301372 [details]
patch

[Chris Dumez]

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 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]

Created attachment 301375 [details]
patch

[Chris Dumez]

Comment on attachment 301375 [details]
patch

lgtm

[Antti Koivisto]

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 on attachment 301381 [details]
patch

lgtm.

[JF Bastien]

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

[Antti Koivisto]

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