1976376 - UAF Crash in [@ mozilla::net::Http2Session::RemoveStreamFromTables]

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[BugBot [:suhaib / :marco/ :calixte]]

Crash report: https://crash-stats.mozilla.org/report/index/d2a28813-2e0c-4c91-ab61-0a4210250702

Reason: EXC_BAD_ACCESS / EXC_I386_GPFLT

Top 10 frames of crashing thread:

0  XUL  PLDHashTable::ComputeKeyHash const  xpcom/ds/PLDHashTable.cpp:479
0  XUL  PLDHashTable::Search const  xpcom/ds/PLDHashTable.cpp:500
1  XUL  nsTHashtable<nsBaseHashtableET<nsPtrHashKey<mozilla::net::nsAHttpTransaction>, RefPtr<mozilla::net::Http2StreamBase> > >::GetEntry const  xpcom/ds/nsTHashtable.h:291
1  XUL  nsRefCountedHashtable<nsPtrHashKey<mozilla::net::nsAHttpTransaction>, RefPtr<mozilla::net::Http2StreamBase> >::Remove  xpcom/ds/nsRefCountedHashtable.h:229
1  XUL  mozilla::net::Http2Session::RemoveStreamFromTables  netwerk/protocol/http/Http2Session.cpp:1410
1  XUL  mozilla::net::Http2Session::CleanupStream  netwerk/protocol/http/Http2Session.cpp:1379
2  XUL  mozilla::net::Http2Session::ReadSegmentsAgain  netwerk/protocol/http/Http2Session.cpp:2706
3  XUL  mozilla::net::nsHttpConnection::OnSocketWritable  netwerk/protocol/http/nsHttpConnection.cpp:1713
4  XUL  mozilla::net::nsHttpConnection::OnOutputStreamReady  netwerk/protocol/http/nsHttpConnection.cpp:2260
4  XUL  {virtual override thunk}  netwerk/protocol/http/nsHttpConnection.cpp

By querying Nightly crashes reported within the last 2 months, here are some insights about the signature:

• First crash report: 2025-07-02
• Process type: Parent
• Is startup crash: No
• Has user comments: No
• Is null crash: No
• Is use after free crash: Yes - all crashes happened on or near an allocator poison value

Comment 1

[Daniel Veditz [:dveditz]]

There are two 142 nightly crashes with this signature that are both UAFs. The Http2Session::RemoveStreamFromTable line in both stacks is just a few lines after some lines that were changed in bug 1974155. But that can't be the regressing bug because it didn't land until the day after the first crash which was in build ID 20250701155448. Maybe it's related to bug 1973772 we were trying to fix?

There are also four crashes on Release 140. These are not crashing on the UAF marker, but they're crashing in the same place and the pointers look like garbage. Maybe it's the same bug?

Comment 2

[Kershaw Chang [:kershaw]]

I think the problem might be that the h2 stream is released on another thread (this may happen for Http2StreamTunnel). I'll write a patch to make sure it's always released on the socket thread.

Comment 3

[Dianna Smith [:diannaS]]

:dveditz it actually could be bug 1974155 because that partly landed in 20250701155448 (D255083) and then the rest landed the next day (D255628). going to mark it as the regressor for tracking purposes. Marking 141 as affected because it was uplifted to beta.
It might not explain the 140 crashes, but I will note that bug 1970656 landed in 140.

Feel free to remove if I misunderstood.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: (secure)

Comment 5

[Kershaw Chang [:kershaw]]

Sorry, I don't think this is a regression. This is more like the crash we tried to fix in bug 1973772.
Bug 1974155 was basically a refactor of how stream is queued and it didn't affect the lifetime of stream.

Comment 6

[Kershaw Chang [:kershaw]]

Comment on attachment 9500021 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not sure. The root cause of this crash is still unclear.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• 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?: Low. This patch is straightforward.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 9500021 [details]
(secure)

Approved to land

Comment 8

[Ryan VanderMeulen [:RyanVM]]

So the ESR branches are also affected? I'm guessing we'll need some other patches nominated for uplift for this to cherry-pick cleanly or branch-specific patches made in that case?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c94907f4ae0f
https://hg.mozilla.org/integration/autoland/rev/a39697e37091
More Http2StreamBase cleanup, r=necko-reviewers,sunil

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8940dac1e294
https://hg.mozilla.org/integration/autoland/rev/c11c7661637f
Revert "Bug 1976376 - More Http2StreamBase cleanup, r=necko-reviewers,sunil" for causing bustages at Http2StreamBase.cpp.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for non-unified build bustages0:

[task 2025-07-22T01:57:22.572+00:00] 01:57:22     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-x86_64-linux-gnu -o Http2StreamBase.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -fstrict-flex-arrays=1 -DNDEBUG=1 -DTRIMMED=1 '-DMOZ_APP_UA_NAME=""' -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DMOZ_SUPPORT_LEAKCHECKING -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/netwerk/protocol/http -I/builds/worker/workspace/obj-build/netwerk/protocol/http -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/netwerk/base -I/builds/worker/checkouts/gecko/netwerk/cookie -I/builds/worker/checkouts/gecko/netwerk/dns -I/builds/worker/checkouts/gecko/netwerk/ipc -I/builds/worker/checkouts/gecko/netwerk/socket/neqo_glue -I/builds/worker/checkouts/gecko/netwerk/url-classifier -I/builds/worker/checkouts/gecko/extensions/auth -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -D_GLIBCXX_USE_CXX11_ABI=0 -fno-rtti -pthread -fno-sized-deallocation -fno-aligned-new -ffunction-sections -fdata-sections -fno-math-errno -fno-exceptions -fPIC -fcrash-diagnostics-dir=/builds/worker/artifacts -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wbitfield-enum-conversion -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtautological-constant-in-range-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wenum-compare-conditional -Wenum-float-conversion -Wno-deprecated-anon-enum-enum-conversion -Wno-deprecated-enum-enum-conversion -Wno-deprecated-this-capture -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wformat -Wformat-security -Wno-psabi -Wthread-safety -Wno-error=builtin-macro-redefined -Wno-vla-cxx-extension -Wno-unknown-warning-option -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/Http2StreamBase.o.pp   /builds/worker/checkouts/gecko/netwerk/protocol/http/Http2StreamBase.cpp
[task 2025-07-22T01:57:22.573+00:00] 01:57:22    ERROR -  /builds/worker/checkouts/gecko/netwerk/protocol/http/Http2StreamBase.cpp:73:16: error: no member named 'components' in namespace 'mozilla'
[task 2025-07-22T01:57:22.573+00:00] 01:57:22     INFO -     73 |       mozilla::components::SocketTransport::Service();
[task 2025-07-22T01:57:22.573+00:00] 01:57:22     INFO -        |       ~~~~~~~~~^

Push with failures

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ec831f04b852
https://hg.mozilla.org/integration/autoland/rev/6e67320c1740
More Http2StreamBase cleanup, r=necko-reviewers,sunil

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6e67320c1740

Comment 14

[Kershaw Chang [:kershaw]]

update: I'll prepare a patch for ESR uplift tomorrow.

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D256973

Comment 16

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Possible crash
• Code covered by automated testing: yes
• 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: This patch is a clenup patch and is straightforward.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: (secure)

Comment 18

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Possible crash
• Code covered by automated testing: yes
• 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: This is a cleanup patch and it's already verified.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 19

[Kershaw Chang [:kershaw]]

Can we avoid uplifting to ESR 128 and 115? There are too many conflicts. I’ve prepared a patch locally, but I’m currently unable to push it to try for testing.

Comment 20

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/9118f2d50ea8
https://hg.mozilla.org/releases/mozilla-beta/rev/eb4342aab6b1

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/a8058026810c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

(In reply to Kershaw Chang [:kershaw] from comment #19)

Can we avoid uplifting to ESR 128 and 115? There are too many conflicts. I’ve prepared a patch locally, but I’m currently unable to push it to try for testing.

It's a sec-high, so in general that means we need to uplift across all supported branches. Given where ESR115/ESR128 are in their lifecycles, it's possible the risk/reward doesn't justify the effort, however. NI Dan for opinions on that.

Comment 23

[Andrew McCreight [:mccr8]]

We've only seen the actual poison crash in 142 and later, so maybe there's some unknown reason it is less of a problem in older versions, despite the basic issue apparently being present for longer.

Comment 24

[Daniel Veditz [:dveditz]]

I agree: there's no evidence the risk is sec-high on the older ESRs. It might require a combination of problems to trigger the UAF, not just the part that was patched here. Wontfix for those is fine.