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

RESOLVED FIXED

Status

()

Core
Plug-ins
--
critical
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({fixed1.9.0.9, fixed1.9.1})

Trunk
x86
All
fixed1.9.0.9, fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.9 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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.
(Assignee)

Comment 2

9 years ago
Of course would be great to find some testcase for these possible problems.
(Assignee)

Comment 3

9 years ago
(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?
(Assignee)

Comment 5

9 years ago
Created attachment 348969 [details] [diff] [review]
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.
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+
(Assignee)

Updated

9 years ago
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.
(Assignee)

Comment 8

9 years ago
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.
(Assignee)

Comment 10

9 years ago
(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+
(Assignee)

Comment 12

9 years ago
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?
(Assignee)

Comment 13

9 years ago
(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+
Comment on attachment 348969 [details] [diff] [review]
nsWeakFrame

a191=beltzner
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs-1.9.1-landing]
Assignee: nobody → Olli.Pettay
(Assignee)

Updated

9 years ago
Keywords: fixed1.9.1
Whiteboard: [needs-1.9.1-landing]
(Assignee)

Comment 15

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c2243275cc44
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?]
(Assignee)

Updated

8 years ago
Attachment #348969 - Flags: approval1.9.0.8?
(Assignee)

Comment 17

8 years ago
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
(Assignee)

Updated

8 years ago
Keywords: fixed1.9.0.8
(Assignee)

Comment 19

8 years ago
Backed out from 1.9.0.8 to see if it caused windows red/orange.
Keywords: fixed1.9.0.8
(Assignee)

Comment 20

8 years ago
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
(Assignee)

Comment 21

8 years ago
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
(Assignee)

Comment 22

8 years ago
The backout didn't help with orange.
(Assignee)

Comment 23

8 years ago
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?
(Assignee)

Comment 25

8 years ago
No. The bug is based on code inspection.

Comment 26

8 years ago
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.

Updated

8 years ago
Depends on: 487345

Comment 30

8 years ago
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

Updated

8 years ago
Flags: in-testsuite-
Depends on: 489988
You need to log in before you can comment on or make changes to this bug.