1574512 - Crash in nr_ice_get_default_local_address

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Not exactly sure how to reproduce this at time-of-filing besides browse some site that must do some WebRTC tracking stuff; but this mechanism of getting the local IP causes nICEr to throw an assert:

 # Child-SP          RetAddr           Call Site
00 000000a2`1a633c40 00007ffc`bb09fe31 ucrtbase!abort+0x4e
01 000000a2`1a633c70 00007ffc`bb0a013b ucrtbase!common_assert_to_stderr<wchar_t>+0x6d
*** WARNING: Unable to verify checksum for C:\mozilla-unified\obj-x86_64-pc-mingw32\dist\bin\xul.dll
02 000000a2`1a633cb0 00007ffc`526654c3 ucrtbase!wassert+0x5b
03 000000a2`1a633ce0 00007ffc`52665094 xul!nr_ice_get_default_local_address+0x1e3 [c:\mozilla-unified\media\mtransport\third_party\nICEr\src\ice\ice_ctx.c @ 658]
04 000000a2`1a634210 00007ffc`4dda260f xul!nr_ice_set_local_addresses+0x2c4 [c:\mozilla-unified\media\mtransport\third_party\nICEr\src\ice\ice_ctx.c @ 742]
05 000000a2`1a63e520 00007ffc`4dd111ff xul!mozilla::NrIceCtx::SetStunAddrs+0x7f [c:\mozilla-unified\media\mtransport\nricectx.cpp @ 565]
06 000000a2`1a63e570 00007ffc`4dd10fd9 xul!mozilla::MediaTransportHandlerSTS::StartIceGathering::<unnamed-tag>::operator()+0x7f [c:\mozilla-unified\media\webrtc\signaling\src\peerconnection\MediaTransportHandler.cpp @ 678]
07 000000a2`1a63e5f0 00007ffc`4dd0b406 xul!mozilla::MozPromise<bool,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,0>::ThenValue<`lambda at c:/mozilla-unified/media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp:662:7',`lambda at c:/mozilla-unified/media/webrtc/signaling/src/peerconnection/MediaTransportHandler.cpp:694:7'>::DoResolveOrRejectInternal+0x29 [c:\mozilla-unified\obj-x86_64-pc-mingw32\dist\include\mozilla\MozPromise.h @ 722]
08 000000a2`1a63e620 00007ffc`4cd6494f xul!mozilla::MozPromise<bool,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,0>::ThenValueBase::ResolveOrRejectRunnable::Run+0x56 [c:\mozilla-unified\obj-x86_64-pc-mingw32\dist\include\mozilla\MozPromise.h @ 399]
09 000000a2`1a63e660 00007ffc`4cd681b6 xul!nsThread::ProcessNextEvent+0xd0f [c:\mozilla-unified\xpcom\threads\nsThread.cpp @ 1214]

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: windbg output.txt

I was wrong about - this is in the socket process; not the content process. (I presume they're not using the same sandbox...)

I placed DebugBreak() at the top of nr_ice_get_default_local_addresses() and nr_ice_get_default_addresses() - then I stepped through those functions successfully a couple times, and then here we do a mov instructions and then suddenly we're inside an assert. I'm not sure what's up with that. I'm going to do a no-opt build and see if I can get something better.

Comment 2

[Alastor Wu [:alwu]]

Hi, Byron,
I saw you worked on this file before, would you mind to take a look on this crash?
Thank you.

Comment 3

[Byron Campen [:bwc]]

So the only plausible assertion I see is here:

https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/media/mtransport/third_party/nICEr/src/ice/ice_ctx.c#633

That can happen if the IP version in the parameters to the function call is invalid somehow. The only place in the codebase that does not pass a valid literal in that parameter is here:

https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/media/mtransport/third_party/nICEr/src/ice/ice_ctx.c#742

So, if we're hitting this assertion, it is probably because the ip_version on target_for_default_local_address_lookup is invalid. This field was added fairly recently. This field is initted here:

https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/media/mtransport/third_party/nICEr/src/ice/ice_ctx.c#801-805

In this code, if the call to nr_str_port_to_transport_addr fails, that field could be left in an inconsistent state; we probably should be clearing it out when that happens. It looks like this will only happen if the passed address is neither a valid IPv4 or IPv6 address. This may also be the ultimate cause of this bug. It looks like this could happen if this code returned something other than an IP address (or empty string):

https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#372-378

We definitely need to fix the error handling in nr_ice_set_target_for_default_local_address_lookup, by freeing and nulling out this field if an error occurs. We probably also want to investigate what exactly the remote host is being set to in the call to httpChannelInternal->GetRemoteAddress in PeerConnectionMedia::SetTargetForDefaultLocalAddressLookup, and see if there's a bug in there too.

Ryan, are you still around to make a quick fix to the error handling?

Comment 4

[Ryan Alderete]

Yes, I am around and can take a look at this.

Comment 5

[Ryan Alderete]

Attachment: Bug 1574512 - Fix error handling in nr_ice_set_target_for_default_local_address_lookup()

If the remote IP address and port number are unable to be converted to a
transport address, the context was incorrectly left with a pointer to zeroed
out memory, which causes nr_ice_get_default_local_address() to abort. Freeing
the address and setting the pointer to null on failure should allow the
fallback to be used to retrieve the default local address.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0dc91d87721b
Fix error handling in nr_ice_set_target_for_default_local_address_lookup() r=bwc

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Looks like this is about to be resolved, but giving it a priority to remove from triage queue.

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0dc91d87721b