Ubuntu

[Patch included] nspluginwrapper has a race condition on NPP_Destroy and may crash Flash

Reported by David Benjamin on 2011-02-24
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
nspluginwrapper (Fedora)
Unknown
Unknown
nspluginwrapper (Ubuntu)
Undecided
Unassigned

Bug Description

Binary package hint: nspluginwrapper

nspluginwrapper has a race condition during NPP_Destroy (called when a tab is closed) that can crash the plugin. The race happens particularly often when another tab has a video playing; I suspect this is because it causes enough traffic over the IPC to delay the processes and trigger the race.

If NPP_Destroy is called by the wrapper process at the same time the viewer (plugin) process makes some call, then, from the plugin's perspective, its call to NPN_InvalidateRect resulted in the plugin instance being destroyed from under its feet. This is, of course, nonsense, so Flash shortly crashes to let us know how silly we are being. :-)

I've written patches for this issue here
https://github.com/davidben/nspluginwrapper/commits/master

Only the second of the two patches is strictly relevant; the other is a separate race I came across in a previous iteration of this patch. It detects when NPP_Destroy is being called at an unsafe point and delays it to another message loop iteration. With the caveat that requests can't be reordered. So, when it must, the patch lies to the wrapper about NPP_Destroy's return values. Any delayed NPSavedData gets discarded. That said, I've never seen Flash use this feature, and the docs do allow the browser to discard them arbitrarily.

The relevant bug in Chromium is here:
http://code.google.com/p/chromium/issues/detail?id=53940

(To be thorough, this is on nspluginwrapper 1.2.2-0ubuntu7 on google-chrome-beta 10.0.648.82-r75062 on Ubuntu maverick. I've also reproduced this crash in Firefox, and a cursory look at Debian stable's nspluginwrapper 1.3.0-1 suggests the bug is there too...)

As far as I can tell, nspluginwrapper no longer has an upstream. If that's not the case, this patch should probably be forward.

David Benjamin (davidben) wrote :

This is the first of two patches in the linked git repo. (The formatting might look off because nspluginwrapper assumes tabs are 4 spaces. It's kind of obnoxious.)

tags: added: patch
David Benjamin (davidben) wrote :

https://bugzilla.redhat.com/show_bug.cgi?id=680279

The patched has made it into Fedora.

Anders Kaseorg (anders-kaseorg) wrote :

Here’s a debdiff including those patches. It also fixes a FTBFS due to a configure test broken by the libxt multiarch transition.

Michael Terry (mterry) wrote :

Thanks, David and Anders! I pushed this to Ubuntu.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package nspluginwrapper - 1.2.2-0ubuntu9

---------------
nspluginwrapper (1.2.2-0ubuntu9) natty; urgency=low

  [ David Benjamin ]
  * Fix a re-entrancy bug with delayed_calls_process and a race condition
    (causing a crash) when a plugin instance is destroyed. (LP: #724587)
    - add debian/patches/008_delayed_calls_process_reentrant.diff
    - add debian/patches/009_npp_destroy_crash.diff
    - update debian/patches/series

  [ Anders Kaseorg ]
  * Fix FTBFS due to configure test broken by libxt multiarch transition.
    - add debian/patches/010_fix_Xt_test.diff
    - update debian/patches/series
  * Update Maintainer to Ubuntu Developers
    <email address hidden>.
 -- Anders Kaseorg <email address hidden> Tue, 29 Mar 2011 14:33:16 -0400

Changed in nspluginwrapper (Ubuntu):
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.