1797674 - Crash in [@ core::slice::index::slice_end_index_len_fail_rt | core::intrinsics::const_eval_select<T>]

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Taegeon Lee]

Crash report: https://crash-stats.mozilla.org/report/index/0aed6483-067d-4874-b901-0f5940221026

MOZ_CRASH Reason: range end index 1252 out of range for slice of length 1024Top 10 frames of crashing thread:

0  xul.dll  MOZ_Crash  mfbt/Assertions.h:261
0  xul.dll  RustMozCrash  mozglue/static/rust/wrappers.cpp:17
1  xul.dll  mozglue_static::panic_hook  mozglue/static/rust/lib.rs:91
2  xul.dll  core::ops::function::Fn::call<void   ../4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248
3  xul.dll  std::panicking::rust_panic_with_hook  library/std/src/panicking.rs:702
4  xul.dll  std::panicking::begin_panic_handler::closure$0  library/std/src/panicking.rs:588
5  xul.dll  std::sys_common::backtrace::__rust_end_short_backtrace<std::panicking::begin_panic_handler::closure_env$0, never$>  library/std/src/sys_common/backtrace.rs:138
6  xul.dll  std::panicking::begin_panic_handler  library/std/src/panicking.rs:584
7  xul.dll  core::panicking::panic_fmt  library/core/src/panicking.rs:142
8  xul.dll  core::slice::index::slice_end_index_len_fail_rt  library/core/src/slice/index.rs:73

Comment 1

[Jens Stutte [:jstutte]]

Based on dom/media/webrtc/transport/mdns_service/src/lib.rs:217 in the crashing stack.

Comment 2

[Andreas Pehrson [:pehrsons]]

From the std::net::UdpSocket::recv_from docs:
: The function must be called with valid byte array buf of sufficient size to hold the message bytes. If a message is too long to fit in the supplied buffer, excess bytes may be discarded.

In this case it read 1252 bytes from the socket but the buffer can only hold 1024.

RFC 6762 says this about mdns packets:

Even when fragmentation is used, a Multicast DNS packet, including IP
and UDP headers, MUST NOT exceed 9000 bytes.

Note that 9000 bytes is also the maximum payload size of an Ethernet
"Jumbo" packet [Jumbo]. However, in practice Ethernet "Jumbo"
packets are not widely used, so it is advantageous to keep packets
under 1500 bytes whenever possible. Even on hosts that normally
handle Ethernet "Jumbo" packets and IP fragment reassembly, it is
becoming more common for these hosts to implement power-saving modes
where the main CPU goes to sleep and hands off packet reception tasks
to a more limited processor in the network interface hardware, which
may not support Ethernet "Jumbo" packets or IP fragment reassembly.

I'll admit I have no prior knowledge of mdns. Nico, would it make sense to make the buffer 1500 or 9000 bytes here? I can put up a patch.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

It should definitely be more than 1024; 1500ish at least. Not sure there's a ton of use in supporting jumbos here, but if this is a stack buffer, that's probably ok. Perhaps start by upping to 1500; I suspect we'll then see this go away

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1797674 - Increase mdns socket buffer to 9000 bytes (the ethernet v2 MTU size). r?ng!

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1797674 - Vendor rust. r?ng!

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Since this is a read buffer, there shouldn't be any harm in allowing for jumbo frames.

Comment 7

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

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:pehrsons, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 8

[Gabriele Svelto [:gsvelto]]

Preemptively adjusting the crash signature based on the changes which will happen in bug 1804025.

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/jobs?repo=try&revision=10bdfd28d0239f47ccbe1d07008d0037a9359ccb

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdbf1ac1bdcc
Increase mdns socket buffer to 9000 bytes (the ethernet v2 MTU size). r=ng
https://hg.mozilla.org/integration/autoland/rev/10f82bcb3aba
Vendor rust. r=ng

Comment 11

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/bdbf1ac1bdcc
https://hg.mozilla.org/mozilla-central/rev/10f82bcb3aba

Comment 12

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

The patch landed in nightly and beta is affected.
:pehrsons, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox108 to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9301300 [details]
Bug 1797674 - Increase mdns socket buffer to 9000 bytes (the ethernet v2 MTU size). r?ng!

Beta/Release Uplift Approval Request

• User impact if declined: Potential parent process crash when setting up a webrtc video call.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This increases the capacity of a read buffer, to be able to handle larger incoming mdns packets. I don't think a patch can get closer to zero risk than this.
• String changes made/needed:
• Is Android affected?: Yes

Comment 14

[Dianna Smith [:diannaS]]

Comment on attachment 9301300 [details]
Bug 1797674 - Increase mdns socket buffer to 9000 bytes (the ethernet v2 MTU size). r?ng!

Switching to Release, since it is already RC week for 108

Approved for 108.0rc2

Comment 15

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-release/rev/d1d409854f73
https://hg.mozilla.org/releases/mozilla-release/rev/bea9aed6d796