Closed Bug 636074 Opened 14 years ago Closed 14 years ago

Crash [@ nsEditor::CreateTxnForDeleteElement]

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: ehsan.akhgari)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical][hardblocker])

Crash Data

Attachments

(4 files, 1 obsolete file)

testcase (crashes Firefox when loaded)
375 bytes, text/html
Details
crash report (mac64 debug)
12.56 KB, text/plain
Details
Part 2: Use nsRefPtr instead of bare ptrs in the remaining CreateTxn callsites
3.59 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 1: fix the crash
7.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
Seems to be a null deref in debug builds but a wild jump in opt builds. Yikes! bp-ad8b7149-a288-4fc6-8502-130ad2110222 Might be related to bug 619287.
blocking2.0: --- → ?
editor = eshan!
Assignee: nobody → ehsan
blocking2.0: ? → final+
Whiteboard: [sg:critical] → [sg:critical][hardblocker]
(In reply to comment #2) > editor = eshan! OK, why is this assigned to me then? I though my h comes before my s! ;-)
This is a regression from bug 619287. We pass an uninitialized pointer to CreateTxnForDeleteElement, and then we swap that into the ref pointer, which makes us try to release a garbage pointer value at the end of that function. This will cause us to dereference the value of that pointer on the stack, which will be dangerous only if the attacker has successfully managed to put what they want at that exact location on the stack before.
Blocks: 619287
And the fix is simple, we should just use forget instead of swap.
(In reply to comment #4) > This is a regression from bug 619287. Oops, I meant bug 633709.
Blocks: 633709
No longer blocks: 619287
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
Attached patch Part 1: fix the crash (obsolete) — — Splinter Review
Attachment #514601 - Flags: review?(roc)
Comment on attachment 514601 [details] [diff] [review] Part 1: fix the crash OK but can we try to reduce the testcase to something near-minimal? Or is that super hard?
Attachment #514601 - Flags: review?(roc) → review+
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Sorry, I keep forgetting that wget'ing the crashtests for security bugs is a really bad idea! ;-)
Attachment #514601 - Attachment is obsolete: true
Attachment #514608 - Flags: review?(roc)
Comment on attachment 514607 [details] [diff] [review] Part 2: Use nsRefPtr instead of bare ptrs in the remaining CreateTxn callsites Kyle wrote exactly what I had in mind as the second part!
Attachment #514607 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/b482c81e9af8 http://hg.mozilla.org/mozilla-central/rev/f80c9b678082
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Landed a follow-up to readjust assertion counts: http://hg.mozilla.org/mozilla-central/rev/317ab332736c
(In reply to comment #15) > Landed a follow-up to readjust assertion counts: > > http://hg.mozilla.org/mozilla-central/rev/317ab332736c This wasn't anything the patches or the test did, this is Bug 635550 striking again.
Crash Signature: [@ nsEditor::CreateTxnForDeleteElement]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: