Closed Bug 462517 Opened 16 years ago Closed 16 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

x86
All
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Keywords: fixed1.9.0.9, fixed1.9.1, Whiteboard: [sg:critical?])

Attachments

(1 file)

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.
Of course would be great to find some testcase for these possible problems.
(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.
dveditz: can you help triage this one?
Attached patch nsWeakFrameSplinter Review
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.
Attachment #348969 - Flags: review?(cbiesinger)
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...
Attachment #348969 - Flags: review?(cbiesinger) → review+
Attachment #348969 - Flags: superreview?(roc)
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.
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.
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.
(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.
(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.
Attachment #348969 - Flags: superreview?(roc) → superreview+
Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

I'm investigating if we should clear the NS_FRAME_EXTERNAL_REFERENCE...
Attachment #348969 - Flags: approval1.9.1?
(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.
Attachment #348969 - Flags: approval1.9.1? → approval1.9.1+
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs-1.9.1-landing]
Assignee: nobody → Olli.Pettay
Keywords: fixed1.9.1
Whiteboard: [needs-1.9.1-landing]
1.9.0 code looks identical. Looks like this doesn't apply to 1.8
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x-
Flags: blocking1.9.0.8+
Whiteboard: [sg:critical?]
Attachment #348969 - Flags: approval1.9.0.8?
Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

The patch applies to 1.9.0.x branch with some fuzz.
Attachment #348969 - Flags: approval1.9.0.8? → approval1.9.0.8+
Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

Approved for 1.9.0.8, a=dveditz for release-drivers
Keywords: fixed1.9.0.8
Backed out from 1.9.0.8 to see if it caused windows red/orange.
Keywords: fixed1.9.0.8
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.
Keywords: fixed1.9.0.8
Backed out again. Not sure if the patch caused orange/red, because one windows
machine turn to red just before the patch was commited.
Keywords: fixed1.9.0.8
The backout didn't help with orange.
I landed this again. The Windows tbox which doesn't randomly turn to orange/red,
stayed green.
Keywords: fixed1.9.0.8
Is there a unit test for this?
No. The bug is based on code inspection.
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.
It's really just another misuse of a dangling pointer.

It could probably be detected via memcheck, although it could be expensive.
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).
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.
Depends on: 487345
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 :(.
That's pretty bad for security issues... One more thing for people to forget to do.
Group: core-security
Flags: in-testsuite-
Depends on: 489988
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: