1910110 - Crash in [@ mozilla::extensions::URLInfo::Host]

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Jan Alexander Steffens [:heftig]]

Crash report: https://crash-stats.mozilla.org/report/index/7d5b7a75-332a-4682-9a2a-022e70240726

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames:

0  libxul.so  mozilla::extensions::URLInfo::Host() const  toolkit/components/extensions/MatchPattern.cpp:130
0  libxul.so  mozilla::extensions::URLInfo::HostAtom() const  toolkit/components/extensions/MatchPattern.cpp:137
0  libxul.so  mozilla::extensions::WebExtensionPolicy::IsRestrictedURI(mozilla::extensions:...  toolkit/components/extensions/WebExtensionPolicy.cpp:541
1  libxul.so  mozilla::extensions::WebExtensionPolicyCore::CanAccessURI(mozilla::extensions...  toolkit/components/extensions/WebExtensionPolicy.cpp:264
2  libxul.so  mozilla::extensions::WebExtensionPolicy::CanAccessURI(mozilla::extensions::UR...  toolkit/components/extensions/WebExtensionPolicy.h:249
2  libxul.so  mozilla::extensions::ChannelWrapper::GetTraceableChannel(mozilla::extensions:...  toolkit/components/extensions/webrequest/ChannelWrapper.cpp:821
3  libxul.so  mozilla::extensions::WebRequestService::GetTraceableChannel(unsigned long, mo...  toolkit/components/extensions/webrequest/WebRequestService.cpp:43
4  libxul.so  mozilla::extensions::ChannelWrapper::GetRegisteredChannel(mozilla::dom::Globa...  toolkit/components/extensions/webrequest/ChannelWrapper.cpp:183
5  libxul.so  mozilla::dom::ChannelWrapper_Binding::getRegisteredChannel(JSContext*, unsign...  s3:gecko-generated-sources-l1:6916ddbe7f83cbf80a4dd275bd1681f37864607d2f6187c299535a3d28bcb23d2ffd6f70a2b20bcb4ec385fd4e50de44e904418b2b308e7e401faed9c6a2efb4/dom/bindings/ChannelWrapperBinding.cpp::1006
6  libxul.so  CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::...  js/src/vm/Interpreter.cpp:491

Marking as sensitive as I suspect this was regressed by bug 1909298, which I cannot access.

Comment 1

[Rob Wu [:robwu]]

Marking as high prio because the patch should have minimal risk. I'll investigate and work on this ASAP.

Comment 2

[Jan Alexander Steffens [:heftig]]

My main profile (the above report) crashes when I navigate to https://www.carnaldish.com/recipes/the-best-homemade-smash-burgers/ . I've been trying to get mozregression --launch 2024-07-26 to crash, without success so far.

Comment 3

[Pascal Chevrel (relman team) -> :pascalc]

This is a top crash on nightly, we had to stop nightly updates.

Comment 4

[Rob Wu [:robwu]]

I think that this happens when the channel is invalid and the getRegisteredChannel call comes in when the original URL cannot be fetched any longer. As a result, FinalURLInfo() returns nullptr (even though that should not happen) and we pass nullptr to CanAccessURI.

I can simulate the same crash trace by changing FinalURLInfo() to nullptr at https://hg.mozilla.org/mozilla-central/rev/3624ccf6ed19#l1.37
and running MOZCONFIG=.mozconfig-debug ./mach test toolkit/components/extensions/test/mochitest/test_ext_webrequest_getSecurityInfo.html --headless --log-mach-verbose

To fix this I am going to also return early when FinalURLInfo() ends up being nullptr.

Comment 5

[Rob Wu [:robwu]]

Backout incoming, fix will be applied to the original patch in bug 1909298.

Comment 6

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1909298

Comment 7

[Rob Wu [:robwu]]

Attachment: Bug 1910110 - Return early when channel URI is void

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/e2a8e98f367a
Return early when channel URI is void r=rpl a=Aryx

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/e2a8e98f367a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Can we come up with a test that would have caught this crash?

Comment 12

[Rob Wu [:robwu]]

Yes we can. I am already working on one and intend to open a public bug to address the root cause more comprehensively.

The reason why I originally skipped the null check in the original patch is that the FinalURLInfo() method had several assertions that suggested that uri cannot be nullptr:

• https://searchfox.org/mozilla-central/rev/0dd776599d18cfc207a44dea24e8595f79a74755/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#594
• https://searchfox.org/mozilla-central/rev/0dd776599d18cfc207a44dea24e8595f79a74755/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#615

The last one is a release assertion.

These assertions are misleading, because it is possible for them to be null, when the channel is no longer valid, as seen in FinalURI(): https://searchfox.org/mozilla-central/rev/0dd776599d18cfc207a44dea24e8595f79a74755/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#959-964

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/08d801735971

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/d2987dd2d6da

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/5994f12d6339

Comment 17

[Rob Wu [:robwu]]

For completeness, the full analysis of how the crash is triggered.

When a http(s) request redirected by an extension to a data:-URL or moz-extension:-URL, ChannelWrapper::SetChannel is called, which saves a reference to the channel with ChannelHolder::SetChannel, with mChannel = do_GetWeakReference(aChannel);.

HTTP channels support nsSupportsWeakReference as needed, but many other channels do not, notably data:-channels (nsDataChannel) and moz-extension:-channels (nsJARChannel). Because of this, channel is nullptr.

Now - when ChannelWrapper.getRegisteredChannel is called (by the built-in webcompat add-on), then ChannelWrapper::GetTraceableChannel is eventually called, which calls FinalURLInfo() with the expectation that it would return the channel's URL. This URI is always expected to exist, except when the underlying wrapped channel is gone. And as we established before, the channel can indeed be gone after a redirect to a non-http channel.

I added test coverage for this along with other tests in bug 1910243. Instead of testing redirection or non-http channels, I am forcing the channel to be garbage-collected. The part that triggers the crash in this bug is in the test task called getRegisteredChannel_after_response_start. Without the patch of this bug, the same crash + stack is observed when ChannelWrapper.getRegisteredChannel is called at line 370 ( https://hg.mozilla.org/integration/autoland/file/2ea9ec869d8907f1150106fb5f1925639a5f4706/toolkit/components/extensions/test/xpcshell/test_ChannelWrapper.js#l366 ).

Comment 18

[Rob Wu [:robwu]]

Tests landed in bug 1910243 as explained comment 17.

Comment 19

[Rob Wu [:robwu]]

Removing bug visibility restrictions - the original security bug that caused the regression was fixed on release over a month ago, and this bug does not have any sensitive information. There is a new report of a crash with a similar stack trace in bug 1918917, and the analysis that I posted in comment 17 would be useful context for discussion.