Bug 113466 – [Mac][WK2] Don’t let plug-ins use System V shared memory

• Component: WebKit2
• Status: RESOLVED FIXED

[Simon Cooper]

Plug-ins hosted by PluginProcess are not allowed to use System V Shared memory.

Give the plug-in something else if they ask for System V shm.

[Simon Cooper]

Created attachment 195467 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 195467 [details]
Patch

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

Looks pretty cool overall, but needs some style fixing, possibly multiple rounds.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:41
> +#ifdef _SHIM_LOG_WITH_ASL
> +#include <asl.h>
> +#endif

Is there a specific reason to use asl here? That's not what we normally do. Would fprintf(stderr, ...) or NSLog work?

The macro needs a more descriptive name (it does not matter that it's WITH_ASL as far as I can tell). How about LOG_SYSV_SHARED_MEMORY_USAGE?

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:133
> +// This needs to C scoped
> +extern "C" {

This comment is not helpful, as it doesn't explain why. In fact, I'm not even sure if this is true that we need C linkage.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:135
> +    // Fake System V shared memory. This is not thread safe.

You could add ASSERT(isMainThread()) to all functions to say this a little more forcefully.

Would be nice to explain what is fake about it. Does it work as shared memory at all?

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:139
> +#ifdef _SHIM_LOG_WITH_ASL
> +    static const char *lid = "PluginProcessShim:";
> +#endif

I'd just put this into each log call, it doesn't help readability or hackability to have this elsewhere.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:149
> +        struct fake_shm *next;
> +        int              refcnt;
> +        key_t            key;
> +        size_t           size;
> +        size_t           alloced_size;
> +        int              shmflag;
> +        int              shmid;
> +        void            *address;

WebKit style: 
- no formatting, please just use a single space between type and name;
- we strongly prefer full words, and camelCase (so, alloced_size would probably be allocatedSize or allocationSize).
- except for Objective-C types, stars stick to types.

        void* address;

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:152
> +    static struct fake_shm *fake_shm_head = NULL;

No "struct" in variable definitions in C++.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:155
> +    static key_t shim_ftok(const char *path, int id)

Misplaced star.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:163
> +    static struct fake_shm *find_by_shmid(int shmid)

No "struct", misplaced star, abbreviations.

These occur many times below, I won't repeat the same comment.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:167
> +        while (shm_p != NULL) {

WebKit style:

while (shm_p) {
}

or better yet, with a more descriptive name than shm_p.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:229
> +        if (shm_p->address != NULL) {
> +            if (shmaddr == 0 || shmaddr == shm_p->address) {

WebKit style says to never compare to 0, so this should be 

if (shm_p->address) {
    if (!shmaddr || shmaddr == shm_p->address)

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:235
> +            shm_p->alloced_size = (shm_p->size+4096) & ~0xFFF;
> +            rval = shm_p->address = mmap((void *)shmaddr, shm_p->alloced_size, PROT_READ|PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);

We always add spaces around binary operations.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:248
> +

Extra empty line.

[Simon Cooper]

Created attachment 196056 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 196056 [details]
Patch

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

This patch still looks great overall, and still needs many style fixes before it can be landed. Someone will need to make these fixes.

> Source/WebKit2/ChangeLog:3
> +        [Mac][WK2] Donât let plug-ins use System V shared memory

This patch is also mangled. Are you using a Unicode aware text editor?

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:129
> +// This needs to C linkage as it is replacing C based system calls.
> +extern "C" {

This comment is not accurate. As evidenced by other functions we have shims for, it is not necessary to use C linkage to interpose.

It's possible that C linkage is needed for some reason, but this comment does not explain it. Ideally, we should be have complete understanding of why we are using particular language constructs, but at the very least, this comment should be removed.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:135
> +    typedef struct shmDescriptor {

This is C++ code, and "typedef struct" should not be used.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:136
> +        struct shmDescriptor *next;

Same concern about incorrect style. This issue is present many times below.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:185
> +        if (descriptorPtr->referenceCount == 0) {

More comparison to zero.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:215
> +            mappedAddress = descriptorPtr->mmapedAddress = mmap((void *)requestedSharedAddress,
> +                                                          descriptorPtr->mmapedSize,
> +                                                          PROT_READ | PROT_WRITE,
> +                                                          MAP_ANON | MAP_PRIVATE, -1, 0);

WebKit coding style forbids this this kind of formatting.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:237
> +        if (descriptorPtr == NULL) {

Comparison to zero.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:270
> +        case IPC_RMID:
> +                errno = EPERM;
> +                goto failed;

Please use 4-space indentation, not 8-space.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:283
> +                outputDescriptor->shm_ctime = outputDescriptor->shm_atime = outputDescriptor->shm_dtime = time(NULL);

We use 0 or nullptr, not NULL.

[Alexey Proskuryakov]

I went ahead and landed this, making necessary changes.

Committed <http://trac.webkit.org/r147500>.

> +// This needs to C linkage as it is replacing C based system calls.
> +extern "C" {

I confirmed that it's not necessary to use C linkage, or to put the code outside WebKit namespace.

My guess is that this didn't work for you because you were adding the code inside #ifndef __LP64__, next to existing overrides.