[Webkit-unassigned] [Bug 113466] Don’t let Plug-ins use System V shared memory

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=113466


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #195467|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Alexey Proskuryakov <ap at webkit.org>  2013-03-27 21:40:53 PST ---
(From update of attachment 195467)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.