Crash with bidi, selection.modify

RESOLVED FIXED in Firefox 23

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jruderman, Assigned: smontagu)

Tracking

(Blocks 2 bugs, 5 keywords)

Trunk
mozilla25
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox22 unaffected, firefox23 fixed, firefox24+ fixed, firefox25+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main23-], crash signature)

Attachments

(4 attachments)

With:
  user_pref("bidi.browser.ui", true);

###!!! ASSERTION: aNode isn't in mRange, or something else weird happened: 'NS_SUCCEEDED(res) && !nodeBefore && !nodeAfter', file content/base/src/nsContentIterator.cpp, line 1430

###!!! ASSERTION: bad index: 'uint32_t(aIndex) < mState.mLength', file nsTextFragment.h, line 162

Crash HasTerminalNewline | nsTextFragment::CharAt
Posted file stack
This is rather annoying to investigate, since I find that it only crashes once per profile and I need to create a new profile for every test run.

FWIW there is a regression range of http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1fb34b07c17&tochange=9db46ddfb517, but it looks as if that just means that bug 765780 exposed an existing bug.
From the stack it looks as if the problem is that the call to nsCaret::InvalidateOutsideCaret from PresShell::CharacterDataChanged is inspecting frame content at a point where the content has changed but the pointers to content cached on the frame haven't been updated yet.
On the other hand, that only refers to the crash with bidi.browser.ui=true; the first assertion occurs even without that pref set, so the "something weird" which is happening needs independent investigation.
The first assertion also shows up in bug 772668, btw.
You can automate the creation of a profile that uses this pref:

mkdir ~/pxa/
echo 'user_pref("bidi.browser.ui", true);' > ~/pxa/prefs.js
firefox -profile ~/pxa/
I'll attach this patch for the crash in the bidi caret code to get feedback on it before I forget, but as I said in the previous comment, the first assertion probably needs to be addressed first, and I'll leave that to the experts on editor and selection kind of stuff :-)
Attachment #768565 - Flags: feedback?(ehsan)
Attachment #768565 - Flags: feedback?(ehsan) → review+
Assignee: nobody → smontagu
Depends on: 765780
Is this going into trunk soon? It will need to have sec-approval? set and the questions there answered.

Since this affects beta and aurora, patches there should be prepared and nominated so we can fix this everywhere?
Comment on attachment 768565 [details] [diff] [review]
Patch for the crash

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The comments and the location of the patch point to the combination of bidi text and text changing, but that doesn't narrow it down very far.

Which older supported branches are affected by this flaw? 23, 24, 25

If not all supported branches, which bug introduced the flaw? Bug 765780 exposed the bug in designMode, but it has probably existed in some form for a long time.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch applies as-is to aurora and beta.

How likely is this patch to cause regressions; how much testing does it need? I think it's not likely to cause regressions, but it could do with testing by someone with experience in editing bidirectional text, with the bidi.ui pref set.
Attachment #768565 - Flags: sec-approval?
Blocks: 895264
Comment on attachment 768565 [details] [diff] [review]
Patch for the crash

Approving for aurora and beta along with sec-approval+. It should go into trunk first but we'll want this in aurora and beta ASAP after that.
Attachment #768565 - Flags: sec-approval?
Attachment #768565 - Flags: sec-approval+
Attachment #768565 - Flags: approval-mozilla-beta+
Attachment #768565 - Flags: approval-mozilla-aurora+
Looks like this landed on trunk, so I'm marking this fixed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
The assertion "aNode isn't in mRange, or something else weird happened" isn't fixed yet. Does that need a new bug, or is it covered by bug 772668?
Oh, right.  It is probably good to split out the assertion into a new bug just to make tracking the security issue easier.  I don't know if the assertion is different here than in the other bug.
Is Firefox 22 unaffected here?
The bug is not reproducible on trunk builds before 2013-04-09, so Firefox 22 should be unaffected, but I haven't tested.
Whiteboard: [adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.