Closed Bug 750575 Opened 12 years ago Closed 12 years ago

Invalid read (from deleted presShell) calling canvas strokeText during full-screen transition

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla16
Tracking Status
firefox13 - wontfix
firefox14 + fixed
firefox15 + fixed
firefox16 --- fixed
firefox-esr10 14+ verified

People

(Reporter: jruderman, Assigned: nrc)

References

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [sg:critical][qa+][advisory-tracking+])

Attachments

(4 files, 3 obsolete files)

1. Set
  user_pref("full-screen-api.allow-trusted-requests-only", false);
2. Load the testcase.

Result: GetPresContext is called on a deleted pres shell, usually causing a crash (which by coincidence looks like a null pres context deref).
Attached file valgrind output
In nsCanvasRenderingContext2DAzure::DrawOrMeasureText

  3188   nsIPresShell* presShell = GetPresShell();
   ...
  3204   nsComputedDOMStyle::GetStyleContextForElement(..., presShell)
   ...
  3248   presShell->GetPresContext(),

GetStyleContextForElement causes a flush that releases the last reference to the pres shell.

Maybe the local |presShell| should be an nsCOMPtr. (If you make a change in nsCanvasRenderingContext2DAzure::DrawOrMeasureText, be sure to make a corresponding change in the non-azure version, since it also crashes.)

Or maybe this is a bug in how full-screen works.  The Mac OS X Lion transition was added recently in bug 639705.  But I first saw this crash on Linux, which doesn't use that transition.
> GetStyleContextForElement causes a flush

Yes.  presShell should be a COMPtr here.  And the GetPresContext() call needs to be null-checked.
Assignee: nobody → ncameron
Attached patch attempt to fix (obsolete) — Splinter Review
I couldn't reproduce the crash on Windows, I tried both a trunk build and current beta, debug and optimised.

Could you let me know if this patch fixes the problem for you? Thanks!
Attachment #620092 - Flags: feedback?(jruderman)
Comment on attachment 620092 [details] [diff] [review]
attempt to fix

Review of attachment 620092 [details] [diff] [review]:
-----------------------------------------------------------------

This should work...

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +2929,5 @@
>      nscoord totalWidthCoord;
>  
>      // calls bidi algo twice since it needs the full text width and the
>      // bounding boxes before rendering anything
> +    NS_ASSERTION(presShell, "presShell is Null");

This assertion is really pointless since we already null-checked it in this method and nothing modifies this local variable.

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +3240,5 @@
>    nscoord totalWidthCoord;
>  
>    // calls bidi algo twice since it needs the full text width and the
>    // bounding boxes before rendering anything
> +  NS_ASSERTION(presShell, "presShell is Null");

Ditto
Attached patch attempt to fix (obsolete) — Splinter Review
Changed the assertions to check the PresContext, I assume they could be Null some how, but this is not checked anywhere. Shouldn't have anything to do with this crash though.
Attachment #620092 - Attachment is obsolete: true
Attachment #620092 - Flags: feedback?(jruderman)
Attachment #620122 - Flags: feedback?(jruderman)
The prescontext can't be null either :-)
Attached patch attempt to fix (obsolete) — Splinter Review
No assertions at all this time.
Attachment #620122 - Attachment is obsolete: true
Attachment #620122 - Flags: feedback?(jruderman)
Attachment #620131 - Flags: feedback?(jruderman)
(In reply to Nick Cameron [:nrc] from comment #5)
> Created attachment 620092 [details] [diff] [review]
> attempt to fix
> 
> I couldn't reproduce the crash on Windows, I tried both a trunk build and
> current beta, debug and optimised.
> 
> Could you let me know if this patch fixes the problem for you? Thanks!

Jesse: could you test the patch on your setup please? Thanks!
Nick: if this is the right fix do we need it on the ESR-10 branch and/or aurora/beta? Or is this a recent regression?
Whiteboard: [sg:critical] → [sg:critical][needs feedback from jruderman]
I'm not sure if it is a recent regression in terms of causing a crash, but I checked the code I am modifying, and it has been around for a long time. The full screen api is changing quite a bit recently, so that could cause this, and it therefore could be recent. Perhaps Jesse could let us know if it occurs on v10 or beta?
Any news on this bug? Should we land the patch?

I have no idea if the patch fixes the problem, but it does correct a bug, and I'm pretty sure it doesn't make anything worse.

It would be great if someone who can reproduce the bug can test the patch, or could help me to reproduce the bug.
Comment on attachment 620131 [details] [diff] [review]
attempt to fix

This patch fixes the bug for me :)
Attachment #620131 - Flags: feedback?(jruderman) → feedback+
Attachment #620131 - Flags: review?(roc)
The testcase doesn't crash ESR for me, but that doesn't necessarily mean ESR is unaffected.
Attached patch patchSplinter Review
no changes (other than setting r=roc), carrying r=roc
Attachment #620131 - Attachment is obsolete: true
Attachment #625521 - Flags: review+
Attachment #625521 - Flags: feedback+
nrc / Jesse: does this crash affect branches?

If so, we probably want to be ready for branch landings ASAP after landing on trunk, and we'd want to schedule those landings carefully, so that we don't end up landing on the branch right before a relase and suddenly breaking stuff, or right after and zero-daying ourselves.

So -- before this lands on trunk, we probably want to know its applicability for our branches and have patches ready to land there within a few days of the trunk-landing.  (if separate patches are needed)

(also, deleting '[needs feedback from jruderman]' since it looks like that was addressed in comment 14 - comment 15)
Whiteboard: [sg:critical][needs feedback from jruderman] → [sg:critical]
What is the risk here?
Absolutely minimal. We're just turning a regular pointer into an nsCOMPtr which adds and releases a reference.
This patch looks like it would apply to the ESR-10 branch as well as aurora/beta. Even if we're not reproducing the crash with this specific testcase on those branches it would be safest to take this patch anyway -- there may be other ways to encourage a PresShell to die prematurely.
Too late to get this in to beta (code freeze is tomorrow) so tracking for aurora & esr 14
Fixed bitrot (by applying patch w/ fuzz) & softened the commit message to be more coy about the exploitability, and pushed:
  https://hg.mozilla.org/mozilla-central/rev/5581c3b2a95c

(Less concerned about Comment 22 now that we're in a better spot in the release cycle.)
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Comment on attachment 625521 [details] [diff] [review]
patch

I'm just going ahead & requesting approval for aurora/beta/esr per Comment 21.

[Approval Request Comment]
Fix Landed on Version: 16
Risk to taking this patch (and alternatives if risky):  Minimal (see comment 20)
User impact if declined: Possible crashiness / exploitability - see comment 21.
String or UUID changes made by this patch: none
Attachment #625521 - Flags: approval-mozilla-esr10?
Attachment #625521 - Flags: approval-mozilla-beta?
Attachment #625521 - Flags: approval-mozilla-aurora?
Verified the crash pre-fix and the lack of crash in the 6/14 nightly trunk build.
Status: RESOLVED → VERIFIED
Comment on attachment 625521 [details] [diff] [review]
patch

Low risk, sg:crit fix. Approved for all branches.
Attachment #625521 - Flags: approval-mozilla-esr10?
Attachment #625521 - Flags: approval-mozilla-esr10+
Attachment #625521 - Flags: approval-mozilla-beta?
Attachment #625521 - Flags: approval-mozilla-beta+
Attachment #625521 - Flags: approval-mozilla-aurora?
Attachment #625521 - Flags: approval-mozilla-aurora+
(In reply to Daniel Holbert [:dholbert] from comment #27)
> https://hg.mozilla.org/releases/mozilla-esr10/rev/65ffec7cdb1d

Sorry, that ESR link was the wrong cset -- here's the correct one:
 https://hg.mozilla.org/releases/mozilla-esr10/rev/c0a3383e56d7
Keywords: qawanted
The attached testcase still crashes Firefox 10.0.6esrpre 2012-06-19. Should this be reopened?
Whiteboard: [sg:critical] → [sg:critical][qa+]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #29)
> The attached testcase still crashes Firefox 10.0.6esrpre 2012-06-19. Should
> this be reopened?

Sorry, latest is actually:
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.5esrpre) Gecko/20120531 Firefox/10.0.5esrpre

I take it we need a new ESR Nightly to be built for me to verify this.
Sorry for the bugspam -- I just realized there is a 10.0.6pre from today in the same folder -- testing it now. Sorry for the confusion.
Retested Firefox 10.0.6esrpre 2012-06-19 and the testcase does not crash. Apologies again for the churn.

Is regressionwindow-wanted still necessary for this bug?
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+][advisory-tracking+]
Taking the lack of follow-up to comment 12 as a "no" to resolving qawanted/regressionwindow-wanted. Please re-add if there is something more QA can do.
Out of curiosity, why not change the other PresShell references in the same files?
(In reply to Mike Hommey [:glandium] from comment #34)
> Out of curiosity, why not change the other PresShell references in the same
> files?

I would guess they should be, but perhaps if they don't need to be, then it is better to avoid the addref/release. I don't really know. roc - do you have an opinion on this?
Group: core-security
You need to log in before you can comment on or make changes to this bug.