Crash SIGSEGV GetCaretFrameForNodeOffset at 0xffffffff (32-bit) or 0x0000ffffffff (64-bit) after malloc failure on nsTextFragment::SetTo
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: sourc7, Assigned: emilio)
Details
(Keywords: csectype-oom, reporter-external, sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main127+][adv-esr115.12+])
Attachments
(11 files)
|
6.98 KB,
text/plain
|
Details | |
|
469 bytes,
text/html
|
Details | |
|
1.01 KB,
patch
|
Details | Diff | Splinter Review | |
|
2.79 KB,
text/plain
|
Details | |
|
48 bytes,
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-release-
|
Details | Review |
|
18.82 KB,
text/plain
|
Details | |
|
1.83 KB,
text/plain
|
Details | |
|
844 bytes,
text/html
|
Details | |
|
48 bytes,
text/x-phabricator-request
|
phab-bot
:
approval-mozilla-beta+
|
Details | Review |
|
48 bytes,
text/x-phabricator-request
|
phab-bot
:
approval-mozilla-esr115+
|
Details | Review |
|
207 bytes,
text/plain
|
Details |
When run testcase with fillMemory and setRangeText, on Firefox official Nightly 32-bit build I notice it able to crash on GetCaretFrameForNodeOffset at 0xffffffff on 32-bit build or 0x0000ffffffff 64-bit build, the crash occur after malloc failure on nsTextFragment::SetTo.
On original testcase I able to crash at GetCaretFrameForNodeOffset as attached log_minidump_00.txt, but it still require many tries to crash reliably, have to tweak the fillMemory code. For now to reproduce the crash reliably we can simulate the malloc failure on nsTextFragment::SetTo and visit the testcase-for-patch.html.
On local build I able to crash as described after checkout on commit
Bug 1860328 - Track nsCaret position at the DOM level
Steps to reproduce:
- Apply nsTextFragment-setTo-failure.patch
- Compile Firefox
- Visit attached testcase-for-patch.html
- Firefox crash on GetCaretFrameForNodeOffset
| Reporter | ||
Comment 1•11 months ago
|
||
| Reporter | ||
Comment 2•11 months ago
|
||
Updated•11 months ago
|
Comment 3•11 months ago
|
||
:sefeng, since you are the author of the regressor, bug 1860328, could you take a look? Also, could you set the severity field?
For more information, please visit BugBot documentation.
Comment 4•11 months ago
|
||
Set release status flags based on info from the regressing bug 1860328
| Reporter | ||
Comment 5•11 months ago
|
||
Updated•11 months ago
|
Updated•11 months ago
|
Comment 6•11 months ago
|
||
Ah, based on the description, this relates to tracking caret position.
| Assignee | ||
Comment 7•11 months ago
|
||
Don't release the text eagerly until we know we have a buffer to put the
new text in.
Updated•11 months ago
|
| Assignee | ||
Comment 8•11 months ago
|
||
I can't remove the graphics-core-security tag, and if I add dom-core-security then I restrict who can see it to people that are on both groups... Can someone remove gfx-core-security and add dom-core-security?
| Assignee | ||
Comment 9•11 months ago
|
||
Not really a regression from that bug. I mean, it is in that particular test-case, but the bug was latent and pretty sure it could be made to crash elsewhere :)
Comment 10•11 months ago
|
||
Updated•11 months ago
|
Updated•11 months ago
|
Comment 11•11 months ago
|
||
Comment 12•11 months ago
|
||
The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox127towontfix.
For more information, please visit BugBot documentation.
| Assignee | ||
Comment 13•11 months ago
|
||
Not sure it's worth uplifting, wdyt?
| Reporter | ||
Comment 14•11 months ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)
Not really a regression from that bug. I mean, it is in that particular test-case, but the bug was latent and pretty sure it could be made to crash elsewhere :)
Thanks Emilio for the patch!
Yes, it's able to crash elsewhere.
I found new testcase, it crash on FindChar code, interestingly I able to control the SEGV address through setRangeText(String.fromCodePoint(990630).repeat(x), the crash address is controllable through the x number.
This crash is also fixed by patch on comment 7.
I'll share the testcase soon.
| Reporter | ||
Updated•11 months ago
|
| Reporter | ||
Comment 15•11 months ago
|
||
| Reporter | ||
Comment 16•11 months ago
|
||
Ok, here the new testcase, the crash address is controllable through crashAddress variable, in this testcase I use 10526871 it will crash at 0x01414144.
However since it use setRangeText and fromCodePoint.repeat more higher the number, more slower to crash the Firefox.
Steps to reproduce:
- Download the nsTextFragment-setTo-failure.patch
- Change the
runCount == 452torunCount == 20(or lower than 30) - Apply the patch then compile Firefox
- Visit attached testcase.FindChar.0x01414144.html
- After couple of seconds, Firefox crash at
0x01414144
Comment 17•11 months ago
|
||
Upgrading to sec-moderate based on the new testcase showing PC control.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)
Not sure it's worth uplifting, wdyt?
Actually we'd like this uplifted just in case someone can build on this and figure out how to trigger the OOM at the right time in a stock Firefox.
Updated•11 months ago
|
| Assignee | ||
Comment 18•11 months ago
|
||
Comment on attachment 9402156 [details]
Bug 1896555 - Fix nsTextFragment OOM handling. r=#dom-core,smaug
Beta/Release Uplift Approval Request
- User impact if declined: Potential security issue in case of OOM.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: none
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Relatively well scoped patch, but might need a rebase to account for
nsStringBuffer->StringBufferrename. Should be trivial tho. - String changes made/needed: none
- Is Android affected?: Yes
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: See above.
- User impact if declined: see above
- Fix Landed on Version: 128
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Very straight-forward patch to code that's very exercised on regular conditions.
Comment 19•11 months ago
|
||
Comment on attachment 9402156 [details]
Bug 1896555 - Fix nsTextFragment OOM handling. r=#dom-core,smaug
Rejecting release uplift request since we don't ship secbugs in dot releases.
This will ride the train with Fx127
Comment 20•11 months ago
|
||
Emilio, the patch doesn't apply to the beta and esr115 branches as I think the work in bug 1892257 conflicts. Could you provide a rebased patch? Thanks
| Assignee | ||
Comment 21•11 months ago
|
||
Don't release the text eagerly until we know we have a buffer to put the
new text in.
Original Revision: https://phabricator.services.mozilla.com/D210616
Updated•11 months ago
|
| Assignee | ||
Comment 22•11 months ago
|
||
Don't release the text eagerly until we know we have a buffer to put the
new text in.
Original Revision: https://phabricator.services.mozilla.com/D210616
Updated•11 months ago
|
Comment 23•11 months ago
|
||
beta Uplift Approval Request
- User impact if declined: comment 0
- Code covered by automated testing: no
- Fix verified in Nightly: yes
- Needs manual QE test: no
- Steps to reproduce for manual QE testing: N/A
- Risk associated with taking this patch: low
- Explanation of risk level: trivial change to oom handling code which is otherwise exercised very often.
- String changes made/needed: none
- Is Android affected?: yes
Comment 24•11 months ago
|
||
esr115 Uplift Approval Request
- User impact if declined: comment 0
- Code covered by automated testing: no
- Fix verified in Nightly: yes
- Needs manual QE test: no
- Steps to reproduce for manual QE testing: N/A
- Risk associated with taking this patch: low
- Explanation of risk level: See other patch and bug.
- String changes made/needed: none
- Is Android affected?: yes
| Assignee | ||
Updated•11 months ago
|
| Assignee | ||
Comment 25•11 months ago
|
||
Provided rebased patches via phab, lmk if that doesn't work for some reason?
Updated•11 months ago
|
Updated•11 months ago
|
Comment 26•11 months ago
|
||
| uplift | ||
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Comment 27•11 months ago
|
||
| uplift | ||
Updated•11 months ago
|
Updated•11 months ago
|
Comment 28•11 months ago
|
||
Updated•11 months ago
|
Updated•11 months ago
|
Updated•2 months ago
|
Description
•