19309 2008-05-29 09:09:05 -0700 uninitialised variable in PluginView 2008-06-08 11:17:35 -0700 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 chpe webkit-unassigned alp darin oldest_to_newest 81846 0 chpe 2008-05-29 09:09:05 -0700 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.) 81847 1 21410 chpe 2008-05-29 09:10:50 -0700 Created attachment 21410 patch 81854 2 21410 darin 2008-05-29 09:50:14 -0700 Comment on attachment 21410 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. 81856 3 alp 2008-05-29 10:01:23 -0700 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. 81860 4 alp 2008-05-29 10:31:38 -0700 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). 81862 5 darin 2008-05-29 10:39:29 -0700 (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? 81870 6 alp 2008-05-29 11:08:08 -0700 (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? 81879 7 darin 2008-05-29 12:07:33 -0700 (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. 82620 8 darin 2008-06-08 11:17:35 -0700 Committed revision 34443. 21410 2008-05-29 09:10:50 -0700 2008-05-29 09:50:14 -0700 patch P text/plain 882 chpe ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MGRhMmJiMC4uNDRmYjMwMCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMSBAQAorMjAwOC0wNS0yNSAgQ2hyaXN0aWFuIFBl cnNjaCAgPGNocGVAZ25vbWUub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogcGx1Z2lucy9QbHVnaW5WaWV3LmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OlBsdWdpblZpZXc6OlBsdWdpblZpZXcpOiBJbml0aWFsaXNlIHRoZSBOUFAncyBwZGF0YSBt ZW1iZXIKKyAgICAgICAgdG8gMC4KKwogMjAwOC0wNS0yNSAgTWFyY28gQmFyaXNpb25lICA8bWFy Y28uYmFyaXNpb25lQGNvbGxhYm9yYS5jby51az4KIAogICAgICAgICBSZXZpZXdlZCBieSBBbHAg VG9rZXIuCmRpZmYgLS1naXQgYS9XZWJDb3JlL3BsdWdpbnMvUGx1Z2luVmlldy5jcHAgYi9XZWJD b3JlL3BsdWdpbnMvUGx1Z2luVmlldy5jcHAKaW5kZXggMDZmNTc5Yi4uOTk1ODk4NiAxMDA2NDQK LS0tIGEvV2ViQ29yZS9wbHVnaW5zL1BsdWdpblZpZXcuY3BwCisrKyBiL1dlYkNvcmUvcGx1Z2lu cy9QbHVnaW5WaWV3LmNwcApAQCAtNTUyLDYgKzU1Miw3IEBAIFBsdWdpblZpZXc6OlBsdWdpblZp ZXcoRnJhbWUqIHBhcmVudEZyYW1lLCBjb25zdCBJbnRTaXplJiBzaXplLCBQbHVnaW5QYWNrYWdl KiBwCiAKICAgICBtX2luc3RhbmNlID0gJm1faW5zdGFuY2VTdHJ1Y3Q7CiAgICAgbV9pbnN0YW5j ZS0+bmRhdGEgPSB0aGlzOworICAgIG1faW5zdGFuY2UtPnBkYXRhID0gMDsKIAogICAgIG1fbWlt ZVR5cGUgPSBtaW1lVHlwZS51dGY4KCk7CiAK