Bug 19309 – uninitialised variable in PluginView

• Component: Plug-ins
• Status: RESOLVED FIXED

[Christian Persch (GNOME) (away; not reading bugmail)]

PluginView's NPP instance's pdata member is uninitialised. I was getting crashes in the totem plugins because of this; attached patch inits the var to 0. (This might have been a problem in the plugin itself, but I think it's better to be resilient against buggy plugins than just crash.)

[Christian Persch (GNOME) (away; not reading bugmail)]

Created attachment 21410 [details]
patch

[Darin Adler]

Comment on attachment 21410 [details]
patch

Isn't it the plug-in's responsibility to initialize pdata if it wants to use it?

I guess there's no harm in this in either case, so r=me.

[Alp Toker]

The Moonlight plugin team also noticed they were relying on structures being zeroed when initialized, since this is Mozilla's behavour (at least on Linux/X11, I understand NPN_MemAlloc zeros as well as allocating while ours doesn't zero). They fixed the assumptions in their code, but I wonder if we should just go ahead and zero all newly-allocated data and structures passed to plugins as a rule rather than patching up issues individually as we discover them.

If plugins in the wild are assuming the allocated data is zeroed and transmitting uninitialised buffers over the network this could lead to security issues even in plugins that seem to work fine.

[Alp Toker]

CC'ing Darin. We should probably skip this patch and match Mozilla's behaviour as I described since plugins are expecting zeroed input (even though the NPAPI docs warn plugin authors not to rely on this).

[Darin Adler]

(In reply to comment #3)
> The Moonlight plugin team also noticed they were relying on structures being
> zeroed when initialized, since this is Mozilla's behavour (at least on
> Linux/X11, I understand NPN_MemAlloc zeros as well as allocating while ours
> doesn't zero). They fixed the assumptions in their code, but I wonder if we
> should just go ahead and zero all newly-allocated data and structures passed to
> plugins as a rule rather than patching up issues individually as we discover
> them.

The NPN_MemAlloc change sounds OK. Any other specific cases?

[Alp Toker]

(In reply to comment #5)
> 
> The NPN_MemAlloc change sounds OK. Any other specific cases?
> 

The WebKit NPP, NPWindow, NPStream we pass to plugins are allocated as part of the C++ class, and the default memory allocator used for objects doesn't zero. There are some attempts to zero the fields, but if you take a look at PluginStream.cpp, the 'headers' field remains uninitialized until PluginStream::startStream(), by which time a plugin may already have tried to access it.

Perhaps it'd be more effective to allocate and initialize these structures explicitly rather than pointing the plugin to an offset in the C++ class?

[Darin Adler]

(In reply to comment #6)
> Perhaps it'd be more effective to allocate and initialize these structures
> explicitly rather than pointing the plugin to an offset in the C++ class?

That sounds like overkill. It's trivial to initialize to 0:

    memset(&x, 0, sizeof(x));

So if we want initialization, then lets add it.

[Darin Adler]

Committed revision 34443.