462517 - nsAsyncInstantiateEvent::Run() uses pointer comparison to check if old frame == new frame

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

The new frame may be located exactly in the same place in the memory (especially 
because of frame arena) and that is why frame == mFrame looks pretty scary.
There is a flush just before the check and that could run scripts or do whatever.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsObjectLoadingContent.cpp&rev=1.88&mark=135-137#124

Timeless has a stack trace which *might* be related to this problem.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Also http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsObjectLoadingContent.cpp&rev=1.88&mark=126-129#124 looks wrong.
The flush may revoke the event.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Of course would be great to find some testcase for these possible problems.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #2)
> Of course would be great to find some testcase for these possible problems.
I need to emphasize "possible". It was just the stack trace which looked
strange after Run() method.

Timeless, would be great if you could post the stack trace.

Comment 4

[Brandon Sterne (:bsterne)]

dveditz: can you help triage this one?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: nsWeakFrame

Biesi, what do you think about this? This should be pretty safe.

I was hoping to be able just check if there is a new mPendingInstantiateEvent,
but that wouldn't handle all the cases, like nsIPluginDocument.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

looks good.

-        new nsAsyncInstantiateEvent(this, aFrame, mContentType, mURI);
+      new nsAsyncInstantiateEvent(this, frame, mContentType, mURI);

I kind of like using 4-space indentation for continuation lines...

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't like the idea of keeping nsWeakFrame in an event --- that could be on the event queue for a while.

Another way to fix this would be to keep a reference to the pending event in nsObjectLoadingContent, and when an nsObjectFrame is destroyed, poke the event to stop it from doing anything when it runs.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Usually nsWeakFrames are stack only, sure, but we do keep them in heap already, 
for example nsEventStateManager does that.

(In reply to comment #7
> Another way to fix this would be to keep a reference to the pending event in
> nsObjectLoadingContent, and when an nsObjectFrame is destroyed, poke the event
> to stop it from doing anything when it runs.
So nsObjectFrame should notify nsObjectloadingContent which should then
notify/clear the event. That sort of manually doing the same what
nsWeakFrame does.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsWeakFrame penalizes the destruction of every frame. That's going to be a problem if we start leaving nsWeakFrames lying around.

> Usually nsWeakFrames are stack only, sure, but we do keep them in heap already, 
> for example nsEventStateManager does that.

Well, that's bad.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #9)
> nsWeakFrame penalizes the destruction of every frame. That's going to be a
> problem if we start leaving nsWeakFrames lying around.
It penalizes the destruction of the frames which have or have had a 
nsWeakFrame, not every frame.

> Well, that's bad.
I changed that to be so, because before nsWeakFrames ESM marked all the
relevant frames to need special destruction. Nowadays nsWeakFrames does the same.

But, I could optimize nsWeakFrames, so that NS_FRAME_EXTERNAL_REFERENCE is cleared when there are no nsWeakFrames for the frame. I probably need a bit in nsFrame to optimize the usual case when there is only one weakframe for the
frame. At some point I even tried to do this, but at that point
weakframes were so rare that it was useless. I could re-check how we behave
nowadays and do the optimization - in a different bug, I hope.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #10)
> (In reply to comment #9)
> > nsWeakFrame penalizes the destruction of every frame. That's going to be a
> > problem if we start leaving nsWeakFrames lying around.
> It penalizes the destruction of the frames which have or have had a 
> nsWeakFrame, not every frame.

Any frame that's had EXTERNAL_REFERENCE, and also any frame that's SELECTED_CONTENT (which is just a hint and we don't clear often).

But OK, it's not as bad as I thought.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

I'm investigating if we should clear the NS_FRAME_EXTERNAL_REFERENCE...

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> I'm investigating if we should clear the NS_FRAME_EXTERNAL_REFERENCE...
...if there are no weak frames for the frame.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

a191=beltzner

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c2243275cc44

Comment 16

[Daniel Veditz [:dveditz]]

1.9.0 code looks identical. Looks like this doesn't apply to 1.8

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

The patch applies to 1.9.0.x branch with some fuzz.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Backed out from 1.9.0.8 to see if it caused windows red/orange.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Checking in content/base/src/nsObjectLoadingContent.cpp;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v  <--  nsObjectLoadingContent.cpp
new revision: 1.94; previous revision: 1.93
done

...and waiting to see if windows stays green. The problem is that windows
tbox is randomly orange/red.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Backed out again. Not sure if the patch caused orange/red, because one windows
machine turn to red just before the patch was commited.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

The backout didn't help with orange.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

I landed this again. The Windows tbox which doesn't randomly turn to orange/red,
stayed green.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Is there a unit test for this?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

No. The bug is based on code inspection.

Comment 26

[Jesse Ruderman]

Could this kind of bug be caught through static analysis or dynamic analysis?

One way to look at this bug is that it's a violation of the rule "program behavior should never depend on where malloc puts things".  But I don't know if that angle helps.

Perhaps an extension to Memcheck could keep a list of the raw pointers to each heap allocation, updated whenever a pointer is copied, and mark all such pointers as "undefined" as soon as free() is called.  Then, doing anything interesting with the old address (such as comparing it to another address) would set off alarms.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's really just another misuse of a dangling pointer.

It could probably be detected via memcheck, although it could be expensive.

Comment 28

[Nicholas Nethercote [inactive]]

Some people have done work on real-time leak detectors that do exactly what Jesse suggested ("keep a list of the raw pointers to each heap allocation, updated whenever a pointer is copied").  There's even a Valgrind tool, Omega, that does this, but I believe it's badly written and not at all reliable.  

The basic idea could be done with Valgrind, but it would be very expensive (more so than Memcheck, I imagine) and it wouldn't be an extension to Memcheck so much as a completely new tool.

An easy extension to Memcheck would be to just mark as undefined the pointer passed to free() (but not any copies of that pointer).

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wouldn't it be easy to alter Memcheck so that for every word-sized comparison, you warn if both operands point to memory that has been dynamically allocated, and one of the operands is a pointer to memory that is now free?

I'm not saying it's worth the overhead, though.

Comment 30

[timeless]

sorry guys, i don't get bugmail for security bugs (this is a personal choice, because i do a lot of reading in public places and don't want to worry about shoulder surfers), so if you need me to comment you actually have to poke me out of band :(.

Comment 31

[Al Billings [:abillings - ex-MoCo]]

That's pretty bad for security issues... One more thing for people to forget to do.