Closed
Bug 1258231
Opened 8 years ago
Closed 8 years ago
crash in nsConsoleService::ClearMessagesForWindowID
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: jesup, Assigned: erahm)
References
Details
(5 keywords, Whiteboard: [post-critsmash-triage][adv-main46+][adv-esr45.1+])
Crash Data
Attachments
(1 file, 1 obsolete file)
1.37 KB,
patch
|
erahm
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr45+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-7db23744-0e54-43f8-b1bf-0b2b52160315. ============================================================= Crash is on a line last touched by froyd in bug 1189829; the bug might pre-exist that change however. UAF signature, with ~1000 crashes in the last week. Likely a sec-crit; certainly a sec-high
Assignee | ||
Comment 1•8 years ago
|
||
So we assert that we're on the main thread [1] which is good enough if anything else accessing the message array is main thread only as well, but that's not the case [2]. I think we need to lock |mLock| to actually guarantee thread safety. [1] http://hg.mozilla.org/releases/mozilla-beta/annotate/fb3494d06dfb/xpcom/base/nsConsoleService.cpp#l77 [2] http://hg.mozilla.org/releases/mozilla-beta/annotate/fb3494d06dfb/xpcom/base/nsConsoleService.cpp#l206
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8732966 -
Flags: review?(nfroyd)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
Also just to be clear, this was a latent issue existing prior to bug 1189829. It was most likely introduced in bug 814497.
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8732966 [details] [diff] [review] Lock while iterating console messages Nathan's out, mccr8 I think you've touched this code before, mind rs'ing?
Attachment #8732966 -
Flags: review?(nfroyd) → review?(continuation)
Updated•8 years ago
|
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → affected
Keywords: csectype-race
Comment 5•8 years ago
|
||
Comment on attachment 8732966 [details] [diff] [review] Lock while iterating console messages Review of attachment 8732966 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Does nsConsoleService::ClearMessages() need this treatment, too? It also accesses mMessages without a lock. I guess it is only called from Reset() where the lock is held and from the dtor which hopefully can't happen when anything else holds it.
Attachment #8732966 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #5) > Comment on attachment 8732966 [details] [diff] [review] > Lock while iterating console messages > > Review of attachment 8732966 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice. Does nsConsoleService::ClearMessages() need this treatment, too? It > also accesses mMessages without a lock. I guess it is only called from > Reset() where the lock is held and from the dtor which hopefully can't > happen when anything else holds it. Yeah I checked that as well and came to the same conclusions.
Comment 7•8 years ago
|
||
Maybe add a comment?
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #7) > Maybe add a comment? Sure.
Assignee | ||
Comment 9•8 years ago
|
||
Update reviewer, added requested comment.
Assignee | ||
Updated•8 years ago
|
Attachment #8732966 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8732986 [details] [diff] [review] Lock while iterating console messages Carrying forward r+. > [Security approval request comment] > How easily could an exploit be constructed based on the patch? Hard for me to assess, I would guess given the prevalence of crashes it would be pretty easy. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Nothing explicit, but it's pretty clear what we're fixing. > Which older supported branches are affected by this flaw? This goes back to 42, so all. > If not all supported branches, which bug introduced the flaw? It affects all, was introduced in bug 814497. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Patch should apply cleanly to all supported branches. > How likely is this patch to cause regressions; how much testing does it need? Not likely to regress, fixes crashes.
Attachment #8732986 -
Flags: sec-approval?
Attachment #8732986 -
Flags: review+
Updated•8 years ago
|
Group: core-security → dom-core-security
Comment 11•8 years ago
|
||
sec-approval+ for trunk. We'll want this on branches too.
tracking-firefox46:
--- → +
tracking-firefox47:
--- → +
tracking-firefox48:
--- → +
tracking-firefox-esr45:
--- → ?
Updated•8 years ago
|
Attachment #8732986 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad759d658a3
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1ad759d658a3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8732986 [details] [diff] [review] Lock while iterating console messages Approval Request Comment [Feature/regressing bug #]: bug 814497 [User impact if declined]: Crashes, UAF. [Describe test coverage new/current, TreeHerder]: Baked on Nightly for a few days. [Risks and why]: None, fixes multithreaded access of console messages. [String/UUID change made/needed]: None.
Flags: needinfo?(erahm)
Attachment #8732986 -
Flags: approval-mozilla-beta?
Attachment #8732986 -
Flags: approval-mozilla-aurora?
Comment 16•8 years ago
|
||
Comment on attachment 8732986 [details] [diff] [review] Lock while iterating console messages Crash fix, sec-high, let's take this for aurora and for beta 6
Attachment #8732986 -
Flags: approval-mozilla-beta?
Attachment #8732986 -
Flags: approval-mozilla-beta+
Attachment #8732986 -
Flags: approval-mozilla-aurora?
Attachment #8732986 -
Flags: approval-mozilla-aurora+
Comment 17•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ebe83472332c
Flags: in-testsuite?
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Comment 19•8 years ago
|
||
Comment on attachment 8732986 [details] [diff] [review] Lock while iterating console messages sec-high, taking it in esr45 too.
Attachment #8732986 -
Flags: approval-mozilla-esr45+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 20•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr45/rev/c1ecd6e886cc
Keywords: checkin-needed
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+][adv-esr45.1+]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•