Closed
Bug 1258231
Opened 9 years ago
Closed 9 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•9 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•9 years ago
|
||
Attachment #8732966 -
Flags: review?(nfroyd)
| Assignee | ||
Updated•9 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
| Assignee | ||
Comment 3•9 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•9 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•9 years ago
|
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → affected
Keywords: csectype-race
Comment 5•9 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•9 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•9 years ago
|
||
Maybe add a comment?
| Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #7)
> Maybe add a comment?
Sure.
| Assignee | ||
Comment 9•9 years ago
|
||
Update reviewer, added requested comment.
| Assignee | ||
Updated•9 years ago
|
Attachment #8732966 -
Attachment is obsolete: true
| Assignee | ||
Comment 10•9 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•9 years ago
|
Group: core-security → dom-core-security
Comment 11•9 years ago
|
||
sec-approval+ for trunk. We'll want this on branches too.
tracking-firefox46:
--- → +
tracking-firefox47:
--- → +
tracking-firefox48:
--- → +
tracking-firefox-esr45:
--- → ?
Updated•9 years ago
|
Attachment #8732986 -
Flags: sec-approval? → sec-approval+
| Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
Keywords: checkin-needed
Comment 13•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•9 years ago
|
Group: dom-core-security → core-security-release
| Assignee | ||
Comment 15•9 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•9 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•9 years ago
|
||
Flags: in-testsuite?
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•9 years ago
|
Comment 19•9 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•9 years ago
|
Keywords: checkin-needed
Comment 20•9 years ago
|
||
Keywords: checkin-needed
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+][adv-esr45.1+]
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•