Closed Bug 1746488 Opened 2 years ago Closed 2 years ago

Fix a number of missing locks in xpcom InputStream code

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
97 Branch
Tracking Flags:
Tracking Status
firefox-esr91 --- wontfix
firefox95 --- wontfix
firefox96 --- wontfix
firefox97 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Keywords: sec-audit, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main97+r])

Attachments

(1 file)

Bug 1746488: xpcom/io cleanup r=smaug
48 bytes, text/x-phabricator-request
Details | Review

There are a number of missing locks in the MultiplexInputStream and NonBlockingAsyncInputStream code.

Most of them have to do with the various IsFoo() methods, though a couple don't.

Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Group: core-security → dom-core-security

xpcom/io cleanup r=smaug
https://hg.mozilla.org/integration/autoland/rev/e38313bed4665401e91ec880e4ae48202b5b845e
https://hg.mozilla.org/mozilla-central/rev/e38313bed466

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox97: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch

Backed out for detected deadlock:

https://hg.mozilla.org/integration/autoland/rev/f2d0516dcc2d1c70b3db8254a2e0711fa9385a58

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=LtGWZI8iQMCUyAcRDLOhRQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=244e1be4013008f11ed5f19bc677295c307862e5
Failure log: https://treeherder.mozilla.org/logviewer?job_id=361977167&repo=autoland

[task 2021-12-21T09:34:33.531Z] 09:34:33     INFO - GECKO(6881) | ###!!! ERROR: Potential deadlock detected:
[task 2021-12-21T09:34:33.534Z] 09:34:33     INFO - GECKO(6881) | === Cyclical dependency starts at
[task 2021-12-21T09:34:33.534Z] 09:34:33     INFO - GECKO(6881) | --- Mutex : nsMultiplexInputStream lock (currently acquired)
[task 2021-12-21T09:34:33.534Z] 09:34:33     INFO - GECKO(6881) |  calling context
[task 2021-12-21T09:34:33.534Z] 09:34:33     INFO - GECKO(6881) |   [stack trace unavailable]
[task 2021-12-21T09:34:33.535Z] 09:34:33     INFO - GECKO(6881) | === Cycle completed at
[task 2021-12-21T09:34:33.536Z] 09:34:33     INFO - GECKO(6881) | --- Mutex : nsMultiplexInputStream lock (currently acquired)
[task 2021-12-21T09:34:33.536Z] 09:34:33     INFO - GECKO(6881) |  calling context
[task 2021-12-21T09:34:33.537Z] 09:34:33     INFO - GECKO(6881) |   [stack trace unavailable]
[task 2021-12-21T09:34:33.538Z] 09:34:33     INFO - GECKO(6881) | ###!!! Deadlock may happen NOW!
[task 2021-12-21T09:34:33.539Z] 09:34:33     INFO - GECKO(6881) | [Parent 6881, Socket Thread] ###!!! ASSERTION: Potential deadlock detected:
[task 2021-12-21T09:34:33.540Z] 09:34:33     INFO - GECKO(6881) | Cyclical dependency starts at
[task 2021-12-21T09:34:33.541Z] 09:34:33     INFO - GECKO(6881) | Mutex : nsMultiplexInputStream lock (currently acquired)
[task 2021-12-21T09:34:33.541Z] 09:34:33     INFO - GECKO(6881) | Cycle completed at
[task 2021-12-21T09:34:33.541Z] 09:34:33     INFO - GECKO(6881) | Mutex : nsMultiplexInputStream lock (currently acquired)
[task 2021-12-21T09:34:33.546Z] 09:34:33     INFO - GECKO(6881) | ###!!! Deadlock may happen NOW!
[task 2021-12-21T09:34:33.553Z] 09:34:33     INFO - GECKO(6881) | : 'Error', file /builds/worker/checkouts/gecko/xpcom/threads/BlockingResourceBase.cpp:245
[task 2021-12-21T09:34:33.553Z] 09:34:33     INFO - GECKO(6881) | #01: NS_DebugBreak [xpcom/base/nsDebugImpl.cpp:429]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #02: mozilla::BlockingResourceBase::CheckAcquire() [xpcom/threads/BlockingResourceBase.cpp:247]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #03: mozilla::OffTheBooksMutex::Lock() [xpcom/threads/BlockingResourceBase.cpp:311]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #04: nsMultiplexInputStream::QueryInterface(nsID const&, void**) [xpcom/io/nsMultiplexInputStream.cpp:175]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #05: void nsCOMPtr<nsISeekableStream>::assign_from_qi<nsIInputStream>(nsQueryInterface<nsIInputStream>, nsID const&) [xpcom/base/nsCOMPtr.h:1190]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #06: nsMIMEInputStream::IsSeekableInputStream() const [netwerk/base/nsMIMEInputStream.cpp:478]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #07: nsMIMEInputStream::QueryInterface(nsID const&, void**) [netwerk/base/nsMIMEInputStream.cpp:108]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #08: nsCOMPtr<nsIInputStreamCallback>::Assert_NoQueryNeeded() [xpcom/base/nsCOMPtr.h:463]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #09: nsCOMPtr<nsIInputStreamCallback>::operator=(nsIInputStreamCallback*) [xpcom/base/nsCOMPtr.h:692]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #10: nsMultiplexInputStream::AsyncWait(nsIInputStreamCallback*, unsigned int, unsigned int, nsIEventTarget*) [xpcom/io/nsMultiplexInputStream.cpp:821]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #11: nsMIMEInputStream::AsyncWait(nsIInputStreamCallback*, unsigned int, unsigned int, nsIEventTarget*) [netwerk/base/nsMIMEInputStream.cpp:280]
[task 2021-12-21T09:34:33.554Z] 09:34:33     INFO - GECKO(6881) | #12: nsMultiplexInputStream::AsyncWaitInternal() [xpcom/io/nsMultiplexInputStream.cpp:880]
[task 2021-12-21T09:34:33.555Z] 09:34:33     INFO - GECKO(6881) | #13: nsBufferedInputStream::AsyncWait(nsIInputStreamCallback*, unsigned int, unsigned int, nsIEventTarget*) [netwerk/base/nsBufferedStreams.cpp:686]
[task 2021-12-21T09:34:33.555Z] 09:34:33     INFO - GECKO(6881) | #14: mozilla::net::nsHttpTransaction::ReadSegments(mozilla::net::nsAHttpSegmentReader*, unsigned int, unsigned int*) [netwerk/protocol/http/nsHttpTransaction.cpp:808]
[task 2021-12-21T09:34:33.555Z] 09:34:33     INFO - GECKO(6881) | #15: mozilla::net::nsHttpConnection::OnSocketWritable() [netwerk/protocol/http/nsHttpConnection.cpp:1838]
[task 2021-12-21T09:34:33.555Z] 09:34:33     INFO - GECKO(6881) | #16: mozilla::net::nsHttpConnection::OnOutputStreamReady(nsIAsyncOutputStream*) [netwerk/protocol/http/nsHttpConnection.cpp:2376]
[task 2021-12-21T09:34:33.555Z] 09:34:33     INFO - GECKO(6881) | #17: mozilla::net::nsHttpConnection::Activate(mozilla::net::nsAHttpTransaction*, unsigned int, int) [netwerk/protocol/http/nsHttpConnection.cpp:705]
[task 2021-12-21T09:34:33.555Z] 09:34:33     INFO - GECKO(6881) | #18: mozilla::net::nsHttpConnectionMgr::DispatchAbstractTransaction(mozilla::net::ConnectionEntry*, mozilla::net::nsAHttpTransaction*, unsigned int, mozilla::net::HttpConnectionBase*, int) [netwerk/protocol/http/nsHttpConnectionMgr.cpp:1598]
[task 2021-12-21T09:34:33.555Z] 09:34:33     INFO - GECKO(6881) | #19: mozilla::net::nsHttpConnectionMgr::DispatchTransaction(mozilla::net::ConnectionEntry*, mozilla::net::nsHttpTransaction*, mozilla::net::HttpConnectionBase*) [netwerk/protocol/http/nsHttpConnectionMgr.cpp:1559]
[task 2021-12-21T09:34:33.562Z] 09:34:33     INFO - GECKO(6881) | #20: mozilla::net::DnsAndConnectSocket::SetupConn(bool, nsresult) [netwerk/protocol/http/DnsAndConnectSocket.cpp:606]
[task 2021-12-21T09:34:33.563Z] 09:34:33     INFO - GECKO(6881) | #21: mozilla::net::DnsAndConnectSocket::OnOutputStreamReady(nsIAsyncOutputStream*) [netwerk/protocol/http/DnsAndConnectSocket.cpp:517]
[task 2021-12-21T09:34:33.572Z] 09:34:33     INFO - GECKO(6881) | #22: mozilla::net::nsSocketOutputStream::OnSocketReady(nsresult) [netwerk/base/nsSocketTransport2.cpp:514]
[task 2021-12-21T09:34:33.573Z] 09:34:33     INFO - GECKO(6881) | #23: mozilla::net::nsSocketTransport::OnSocketReady(PRFileDesc*, short) [netwerk/base/nsSocketTransport2.cpp:2060]
[task 2021-12-21T09:34:33.575Z] 09:34:33     INFO - GECKO(6881) | #24: mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*) [netwerk/base/nsSocketTransportService2.cpp:1380]
[task 2021-12-21T09:34:33.576Z] 09:34:33     INFO - GECKO(6881) | #25: mozilla::net::nsSocketTransportService::Run() [netwerk/base/nsSocketTransportService2.cpp:1152]
[task 2021-12-21T09:34:33.577Z] 09:34:33     INFO - GECKO(6881) | #26: {virtual override thunk({offset(-32)}, mozilla::net::nsSocketTransportService::Run())} [/builds/worker/workspace/build/application/firefox/libxul.so + 0x4852e7f]
[task 2021-12-21T09:34:33.578Z] 09:34:33     INFO - GECKO(6881) | #27: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1178]
[task 2021-12-21T09:34:33.579Z] 09:34:33     INFO - GECKO(6881) | #28: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:467]
[task 2021-12-21T09:34:33.580Z] 09:34:33     INFO - GECKO(6881) | #29: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:301]
[task 2021-12-21T09:34:33.581Z] 09:34:33     INFO - GECKO(6881) | #30: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:331]
[task 2021-12-21T09:34:33.589Z] 09:34:33     INFO - GECKO(6881) | #31: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:307]
[task 2021-12-21T09:34:33.590Z] 09:34:33     INFO - GECKO(6881) | #32: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:393]
[task 2021-12-21T09:34:33.604Z] 09:34:33     INFO - GECKO(6881) | #33: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:204]
[task 2021-12-21T09:34:33.605Z] 09:34:33     INFO - GECKO(6881) | #34: ??? [/lib/x86_64-linux-gnu/libpthread.so.0 + 0x76db]
[task 2021-12-21T09:34:33.606Z] 09:34:33     INFO - GECKO(6881) | #35: clone [/lib/x86_64-linux-gnu/libc.so.6 + 0x121a3f]
[task 2021-12-21T09:34:33.607Z] 09:34:33     INFO - GECKO(6881) | #36: ??? (???:???)
Status: RESOLVED → REOPENED
status-firefox97: fixed → ---
Flags: needinfo?(rjesup)
Resolution: FIXED → ---
Target Milestone: 97 Branch → ---
Flags: needinfo?(rjesup)

xpcom/io cleanup r=smaug
https://hg.mozilla.org/integration/autoland/rev/d08bb1758f659e6bd7e4172967abe0e04d35fc58
https://hg.mozilla.org/mozilla-central/rev/d08bb1758f65

Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
status-firefox97: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main97+r]

I noticed this has been backed out (~7 weeks after it landed); what's the issue? If we're backing it out, it will need to be backed out of beta and release as well; it was fixed in 97

Flags: needinfo?(ctuns)

Redirecting Ni to Aryx.

Flags: needinfo?(ctuns) → needinfo?(aryx.bugmail)

Both files are unchanged since the change in comment 4 landed on 2022-01-04. Which changeset got backed out by which other changeset?

Flags: needinfo?(aryx.bugmail) → needinfo?(rjesup)

This was a confusion in Phab triggered by someone doing a major update to the 'elm' repro; any backouts in the repo change history that was updated on elm caused Phab to re-open the changes and say it was backed out. Caused by a setting in phab getting reset, apparently.

Flags: needinfo?(rjesup)
Regressions: 1758115
No longer regressions: 1758115
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: