Closed
Bug 1379631
Opened 7 years ago
Closed 7 years ago
Hit MOZ_CRASH(nsCORSListenerProxy not thread-safe) at nsISupportsImpl.cpp:43
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
People
(Reporter: cbook, Assigned: dragana)
References
()
Details
(Keywords: assertion, csectype-race, sec-high, Whiteboard: [necko-active])
Attachments
(3 files, 3 obsolete files)
194.45 KB,
text/plain
|
Details | |
864 bytes,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
7.90 KB,
patch
|
dragana
:
review+
|
Details | Diff | Splinter Review |
Found by bughunter on https://www.pandora.com/station/play/1915163060293903816 Bughunter run into this 14 seconds after loading this Hit MOZ_CRASH(nsCORSListenerProxy not thread-safe) at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/xpcom/base/nsISupportsImpl.cpp:43 Unable to read VR Path Registry from C:\Users\mozauto\AppData\Local\openvr\openvrpaths.vrpath [GPU 6260] WARNING: Shutting down GPU process early due to a crash!: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/gfx/ipc/GPUParent.cpp, line 399 marking s-s- because of the experience with other not thread safe bugs :)
Reporter | ||
Comment 1•7 years ago
|
||
andrew: do you know who could take a look ?
Flags: needinfo?(continuation)
Comment 2•7 years ago
|
||
This looks like another case where a class isn't using threadsafe refcounting and is being used by nsUnknownDecoder.
Blocks: 1365519
Component: General → Networking: HTTP
Flags: needinfo?(continuation)
Keywords: csectype-race,
sec-high
Updated•7 years ago
|
Flags: needinfo?(valentin.gosu)
Updated•7 years ago
|
Assignee: nobody → dd.mozilla
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active]
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #2) > This looks like another case where a class isn't using threadsafe > refcounting and is being used by nsUnknownDecoder. once again nsUnknownDecoder is not to blame here. nsUnknownDecoder does not use any other listener. Each listener can veto on decision to be retargeted on a different thread. Why people implement nsIThreadRetargetableStreamListener, do not veto on retargeting and in the same time do not implement threadsafe ref counting, I do not know. I am just wondering why this did not pop up earlier, nsUnknownDecoder is used very rarely.
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8885329 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 5•7 years ago
|
||
i forgot to refresh the patch. sorry for noise.
Attachment #8885329 -
Attachment is obsolete: true
Attachment #8885329 -
Flags: review?(honzab.moz)
Attachment #8885330 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=031cb717dc3b10dd7c4390786c86e13542c934ef
Comment 7•7 years ago
|
||
Comment on attachment 8885330 [details] [diff] [review] bug_1379631.patch Review of attachment 8885330 [details] [diff] [review]: ----------------------------------------------------------------- Dropping r based on the try results like: https://treeherder.mozilla.org/logviewer.html#?job_id=113375055&repo=try&lineNumber=10762 the parent push looks ok, tho. ::: netwerk/protocol/http/nsCORSListenerProxy.cpp @@ +656,5 @@ > + nsCOMPtr<nsIStreamListener> listener; > + { > + MutexAutoLock lock(mMutex); > + listener = mOuterListener; > + mOuterListener = nullptr; swap/forget? ::: netwerk/protocol/http/nsCORSListenerProxy.h @@ +114,5 @@ > bool mInited; > #endif > + > + // only locking mOuterListener, because it can be used on different threads. > + // We guarantee that ONStartRequest, OnDataAvailable and OnStopReques will be "ON" @@ +115,5 @@ > #endif > + > + // only locking mOuterListener, because it can be used on different threads. > + // We guarantee that ONStartRequest, OnDataAvailable and OnStopReques will be > + // called in order, but to make tsan happy we will lock mOuterListener. trailing white
Attachment #8885330 -
Flags: review?(honzab.moz)
Updated•7 years ago
|
Group: core-security → network-core-security
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #7) > Comment on attachment 8885330 [details] [diff] [review] > bug_1379631.patch > > Review of attachment 8885330 [details] [diff] [review]: > ----------------------------------------------------------------- > > Dropping r based on the try results like: > > https://treeherder.mozilla.org/logviewer. > html#?job_id=113375055&repo=try&lineNumber=10762 > FetchDrive is the next one :) implements nsIThreadRetargetableStreamListener and declares NS_DECL_ISUPPORTS
Comment 9•7 years ago
|
||
Comment on attachment 8885330 [details] [diff] [review] bug_1379631.patch Review of attachment 8885330 [details] [diff] [review]: ----------------------------------------------------------------- Then this is r+ :)
Attachment #8885330 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
Version: unspecified → 56 Branch
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8885330 -
Attachment is obsolete: true
Attachment #8892416 -
Flags: review+
Assignee | ||
Comment 11•7 years ago
|
||
I was hoping that bug 1380255 will be fix before 56 goes to beta :( I will turn off OMT delivery for FetchDriver until bug 1380255 gets fixed.
Attachment #8892417 -
Flags: review?(honzab.moz)
Comment hidden (obsolete) |
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8892416 [details] [diff] [review] bug_1379631.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Actually we call this functions on different threads but we guaranty that they are called sequentially (the next will not start before the previous ends, this is guaranteed). So probably not possible to exploit. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Which older supported branches are affected by this flaw? none If not all supported branches, which bug introduced the flaw? only 56 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? How likely is this patch to cause regressions; how much testing does it need? not likely.
Attachment #8892416 -
Flags: sec-approval?
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8892416 -
Attachment is obsolete: true
Attachment #8892416 -
Flags: sec-approval?
Attachment #8892420 -
Flags: review+
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8892420 [details] [diff] [review] bug_1379631.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Actually we call this functions on different threads but we guaranty that they are called sequentially (the next will not start before the previous ends, this is guaranteed). So probably not possible to exploit. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Which older supported branches are affected by this flaw? none If not all supported branches, which bug introduced the flaw? only 56 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? How likely is this patch to cause regressions; how much testing does it need? not likely.
Attachment #8892420 -
Flags: sec-approval?
Comment 16•7 years ago
|
||
Comment on attachment 8892420 [details] [diff] [review] bug_1379631.patch Only affects trunk, so it can land w/o sec-approval.
Attachment #8892420 -
Flags: sec-approval?
Updated•7 years ago
|
Attachment #8892417 -
Flags: review?(honzab.moz) → review+
Comment 17•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/581c2f16e306 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5964aaf117
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 18•7 years ago
|
||
Thank you Honza and Ryan
https://hg.mozilla.org/mozilla-central/rev/581c2f16e306 https://hg.mozilla.org/mozilla-central/rev/7e5964aaf117
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Group: network-core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•