Bug 189741 – [Linux] Use memfd_create when available in SharedMemory implementation

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

[Carlos Garcia Campos]

Instead of shm_open.

[Carlos Garcia Campos]

Created attachment 350101 [details]
Patch

[Michael Catanzaro]

Comment on attachment 350101 [details]
Patch

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

In this case, I would check for sys/memfd.h instead of linux/memfd.h and use memfd_create() directly if it's available (glibc 2.27 or newer) rather than using syscall(). Then you don't have to check for EINTR or ENOSYS.

This way works too, of course.

> Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:128
> +        if (errno == ENOSYS)
> +            return fileDescriptor;

Surely this isn't right? You should just remove this condition and fall through to isMemFdAvailable = false, right?

> Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:154
> -        WTFLogAlways("Failed to create shared memory file %s: %s", tempName.data(), strerror(errno));
> +        WTFLogAlways("Failed to create shared memory file: %s", strerror(errno));

Drop the word "file"

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 350101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350101&action=review
> 
> In this case, I would check for sys/memfd.h instead of linux/memfd.h and use
> memfd_create() directly if it's available (glibc 2.27 or newer) rather than
> using syscall(). Then you don't have to check for EINTR or ENOSYS.
> 
> This way works too, of course.

I'm assuming this is available in more systems, no?

> > Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:128
> > +        if (errno == ENOSYS)
> > +            return fileDescriptor;
> 
> Surely this isn't right? You should just remove this condition and fall
> through to isMemFdAvailable = false, right?

Yes, I changed this several times and ended up with the wrong check. Good catch!

> > Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:154
> > -        WTFLogAlways("Failed to create shared memory file %s: %s", tempName.data(), strerror(errno));
> > +        WTFLogAlways("Failed to create shared memory file: %s", strerror(errno));
> 
> Drop the word "file"

Sure.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #3)
> I'm assuming this is available in more systems, no?

The syscall was added years before the glibc interface, but so what? It's nicer to use the glibc function, and there is a fallback, after all. syscall() is used in the sandbox code because it's really needed there, but here it's not: there is a fallback.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> (In reply to Carlos Garcia Campos from comment #3)
> > I'm assuming this is available in more systems, no?
> 
> The syscall was added years before the glibc interface, but so what? It's
> nicer to use the glibc function, and there is a fallback, after all.
> syscall() is used in the sandbox code because it's really needed there, but
> here it's not: there is a fallback.

I still don't understand why sandbox code can't use SharedMemory.

[Carlos Garcia Campos]

Committed r237922: <https://trac.webkit.org/changeset/237922>

[Radar WebKit Bug Importer]

<rdar://problem/45873944>