Closed Bug 548441 Opened 10 years ago Closed 10 years ago

valgrind: writing freed memory in nsNPAPIPlugin.cpp

Categories

(Core :: Plug-ins, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jseward, Assigned: jseward)

References

(Blocks 1 open bug)

Details

(Keywords: valgrind, Whiteboard: [sg:nse?])

Attachments

(1 file)

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)
Whiteboard: valgrind
Whiteboard: valgrind
> 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?
blocking2.0: --- → ?
Whiteboard: valgrind
Whiteboard: valgrind
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.
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?
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.
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?
Whiteboard: [sg:moderate]
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.
Whiteboard: [sg:moderate] → [sg:nse?]
We should just remove the line that sets pstream->ndata here, it's meaningless because we're deleting pstream.
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.
pstream *should* always be a subobject. We can assert this and find out...
Attachment #431398 - Flags: review?(bzbarsky)
Attachment #431398 - Flags: review?(bzbarsky) → review+
Assignee: nobody → jseward
Pushed http://hg.mozilla.org/mozilla-central/rev/b152283bd43b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
blocking2.0: ? → final+
Group: core-security
You need to log in before you can comment on or make changes to this bug.