548441 - valgrind: writing freed memory in nsNPAPIPlugin.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Julian Seward [:jseward]]

mozilla-central of a few hours back, x86_64-Linux,
mochitest-plain modules/plugin/test/test_pluginstream_newstream.html

gives the error shown below.

  1230     nsNPAPIStreamWrapper* wrapper = (nsNPAPIStreamWrapper *)pstream->ndata;
  1231     NS_ASSERTION(wrapper, "null wrapper");
  1232
  1233     if (!wrapper)
  1234       return NPERR_INVALID_PARAM;
  1235
  1236     // This will release the wrapped nsIOutputStream.
  1237     delete wrapper;
  1238     pstream->ndata = nsnull;

It's complaining that the object deleted at 1237 is then written to at
1238.  This is very strange, unless wrapper and pstream are the same
object.  In fact, from a bit of debug printing, they aren't the same
object, but nearly: it seems like pstream is a sub-object within
wrapper.  wrapper is an object of size 72 bytes at 0x...170, and
pstream is 0x...188, that is, 24 bytes inside wrapper.

How to repro:

DISPLAY=:1.0 make -C ff-opt mochitest-plain \
  TEST_PATH=modules/plugin/test/test_pluginstream_newstream.html \
  EXTRA_TEST_ARGS='--debugger=valgrind --setpref=javascript.options.jit.chrome=false --setpref=javascript.options.jit.content=false'


Invalid write of size 8
   at 0x5E83809: mozilla::plugins::parent::_destroystream(_NPP*, _NPStream*, short) (nsNPAPIPlugin.cpp:1238)
   by 0x6052705: mozilla::plugins::PluginStreamParent::NPN_DestroyStream(short) (PluginStreamParent.cpp:93)
   by 0x6052720: mozilla::plugins::PluginStreamParent::Answer__delete__(short const&, bool const&) (PluginStreamParent.cpp:83)
   by 0x60697C0: mozilla::plugins::PPluginStreamParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginStreamParent.cpp:196)
   by 0x605C4D9: mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginModuleParent.cpp:434)
   by 0x6056E51: mozilla::ipc::RPCChannel::DispatchIncall(IPC::Message const&) (RPCChannel.cpp:425)
   by 0x6057D41: mozilla::ipc::RPCChannel::Call(IPC::Message*, IPC::Message*) (RPCChannel.cpp:259)
   by 0x60684EB: mozilla::plugins::PBrowserStreamParent::Call__delete__(mozilla::plugins::PBrowserStreamParent*, short const&, bool const&) (PBrowserStreamParent.cpp:188)
   by 0x6044071: mozilla::plugins::PluginInstanceParent::NPP_DestroyStream(_NPStream*, short) (PluginInstanceParent.cpp:663)
   by 0x5E89047: nsNPAPIPluginStreamListener::CleanUpStream(short) (nsNPAPIPluginInstance.cpp:278)
   by 0x5E89142: nsNPAPIPluginStreamListener::OnStopBinding(nsIPluginStreamInfo*, unsigned int) (nsNPAPIPluginInstance.cpp:803)
   by 0x5E8FD96: nsPluginStreamListenerPeer::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (nsPluginHost.cpp:1499)
 Address 0x18953050 is 32 bytes inside a block of size 72 free'd
   at 0x4C24E4F: operator delete(void*) (vg_replace_malloc.c:387)
   by 0x5E83808: mozilla::plugins::parent::_destroystream(_NPP*, _NPStream*, short) (nsNPAPIPlugin.cpp:1237)
   by 0x6052705: mozilla::plugins::PluginStreamParent::NPN_DestroyStream(short) (PluginStreamParent.cpp:93)
   by 0x6052720: mozilla::plugins::PluginStreamParent::Answer__delete__(short const&, bool const&) (PluginStreamParent.cpp:83)
   by 0x60697C0: mozilla::plugins::PPluginStreamParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginStreamParent.cpp:196)
   by 0x605C4D9: mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginModuleParent.cpp:434)
   by 0x6056E51: mozilla::ipc::RPCChannel::DispatchIncall(IPC::Message const&) (RPCChannel.cpp:425)
   by 0x6057D41: mozilla::ipc::RPCChannel::Call(IPC::Message*, IPC::Message*) (RPCChannel.cpp:259)
   by 0x60684EB: mozilla::plugins::PBrowserStreamParent::Call__delete__(mozilla::plugins::PBrowserStreamParent*, short const&, bool const&) (PBrowserStreamParent.cpp:188)
   by 0x6044071: mozilla::plugins::PluginInstanceParent::NPP_DestroyStream(_NPStream*, short) (PluginInstanceParent.cpp:663)
   by 0x5E89047: nsNPAPIPluginStreamListener::CleanUpStream(short) (nsNPAPIPluginInstance.cpp:278)
   by 0x5E89142: nsNPAPIPluginStreamListener::OnStopBinding(nsIPluginStreamInfo*, unsigned int) (nsNPAPIPluginInstance.cpp:803)

Comment 1

[Boris Zbarsky [:bzbarsky]]

> it seems like pstream is a sub-object within wrapper

That's exactly what it is.  nsNPAPIStreamWrapper has a NPStream (object, not pointer) member.

The order of lines 1237 and 1238 should just be switched, I think.  Or line 1238 should just go away.

Doug, it looks like you added that line in bug 53363.  Any idea why?

Comment 2

[Doug Turner (:dougt)]

I think that ndata was being checked for null in other places, so after we delete, we must ensure that it is null.

bz, i agree with you; switching the lines is probably okay.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The thing is, once we delete then someone else can write over that memory, so if other places are checking it after we delete it we have a serious problem, no?

Comment 4

[Doug Turner (:dougt)]

according to the docs, ndata is ours and no one should use it but us.  looking at mxr, it doesn't look like any uses ndata after destroy, right.  if we kill line 1238 and crash, that is the serious problem you mention and we should fix that.

Comment 5

[Daniel Veditz [:dveditz]]

But destroy will be called by the plugins themselves. Hopefully they won't keep using their pstream after destroying it, but what if they have an issue and call us again with the same one? Would it really hurt to defensively null out ndata before deleting the wrapper?

Comment 6

[Daniel Veditz [:dveditz]]

I'm guessing horribly with sg:moderate, it's hard to see how this could lead to a working exploit. Even if on some busy multi-core machine the deleted object got immediately reused between those two lines it's hard to imagine how stomping a null at a random spot in someone else's object could be controlled in a useful way.

Comment 7

[Benjamin Smedberg]

We should just remove the line that sets pstream->ndata here, it's meaningless because we're deleting pstream.

Comment 8

[Julian Seward [:jseward]]

Switching the order of the two lines seems the more conservative
solution as it makes no assumptions about the relative addresses
of pstream and wrapper.  Deleting the assignment to pstream->ndata
is only safe if pstream is always a subobject of wrapper.

Comment 9

[Benjamin Smedberg]

pstream *should* always be a subobject. We can assert this and find out...

Comment 10

[Julian Seward [:jseward]]

Attachment: proposed fix

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b152283bd43b