1689867 - Crash in [@ remote::startup::handler::RemoteAgentHandler::allocate::Handle] when --remote-debugging-port has a non-number argument

Status: RESOLVED INVALID

(Remote Protocol :: Agent, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:87.0) Gecko/20100101 Firefox/87.0 ID:20210127093943

When starting Firefox like the following a crash will happen during startup:

/Applications/FirefoxNightly.app/Contents/MacOS/firefox --profile _a/profile --remote-debugging-port http://example.com

Reason is that this argument requires a number, but doesn't correctly check for the type:

https://searchfox.org/mozilla-central/rev/0a489e67575540f5aeb968208ae03ff17eb71e94/remote/startup/handler.rs#71-73

Crash report: https://crash-stats.mozilla.org/report/index/119276e9-2819-4004-b2ff-9b37a0210131

MOZ_CRASH Reason: explicit panic

Top 10 frames of crashing thread:

0 XUL RustMozCrash mozglue/static/rust/wrappers.cpp:17
1 XUL mozglue_static::panic_hook mozglue/static/rust/lib.rs:89
2 XUL core::ops::function::Fn::call /builds/worker/fetches/rustc/lib/rustlib/src/rust/library/core/src/ops/function.rs:70
3 XUL std::panicking::rust_panic_with_hook library/std/src/panicking.rs:597
4 XUL std::panicking::begin_panic::{{closure}} /builds/worker/fetches/rustc/lib/rustlib/src/rust/library/std/src/panicking.rs:522
5 XUL std::sys_common::backtrace::__rust_end_short_backtrace /builds/worker/fetches/rustc/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:141
6 XUL std::panicking::begin_panic /builds/worker/fetches/rustc/lib/rustlib/src/rust/library/std/src/panicking.rs:521
7 XUL remote::startup::handler::RemoteAgentHandler::allocate::Handle remote/startup/handler.rs:39
8 XUL nsCommandLine::EnumerateHandlers toolkit/components/commandlines/nsCommandLine.cpp:466
9 XUL nsCommandLine::Run toolkit/components/commandlines/nsCommandLine.cpp:521

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that this is interacting with XPCOM I wonder how Rust code should behave in such a situation. What should the method return, or output to better handle such a situation? I haven't found other command line parsing code in Rust on mozilla-central, so Nika do you have an advice? Thanks.

Comment 2

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

Closing because no crashes reported for 12 weeks.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #1)

Given that this is interacting with XPCOM I wonder how Rust code should behave in such a situation. What should the method return, or output to better handle such a situation? I haven't found other command line parsing code in Rust on mozilla-central, so Nika do you have an advice? Thanks.

Nika, I assume this needinfo got lost. As such requesting it again in the hope you can give some advice. Thanks.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

According to the docs for the nsICommandLineHandler interface, if you return NS_ERROR_ABORT when handling command lines for launching the application, it will quit the application (https://searchfox.org/mozilla-central/rev/cca1566127a2fcc013e9c09f9d90ed70df2250a4/toolkit/components/commandlines/nsICommandLineHandler.idl#39-41). In your code this case I think you'd just need to return Err(NS_ERROR_ABORT) here: https://searchfox.org/mozilla-central/rev/cca1566127a2fcc013e9c09f9d90ed70df2250a4/remote/components/rust/src/handler.rs#64.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Nika! I wanted to get this implemented and checked again how the crash looks like these days, but it doesn't happen anymore. Instead I see the following output:

➜ mach run --remote-debugging-port http://example.com
 0:01.52 opt/dist/Nightly.app/Contents/MacOS/firefox --remote-debugging-port http://example.com -no-remote -foreground -profile opt/tmp/profile-default
firefox: invalid port: invalid digit found in string

Nika, when using Err(NS_ERROR_ABORT) I'm not able to pass-back details about the failure. So shall we also keep the call to fatalln!() and print the additional failure message? I think that's somewhat helpful to users.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)

Nika, when using Err(NS_ERROR_ABORT) I'm not able to pass-back details about the failure. So shall we also keep the call to fatalln!() and print the additional failure message? I think that's somewhat helpful to users.

If you want to do a clean exit, you could also print the message to stderr with eprintln!( or similar, and then return NS_ERROR_ABORT to continue exiting normally. Main risk of fatalln! is that I'm guessing it panic!s, and could end up looking like a crash on crash-stats, which isn't optimal. It would probably be nicer to print out the error message with eprintln!( and then exit using the normal XPCOM method to avoid the potential crash report.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #6)

If you want to do a clean exit, you could also print the message to stderr with eprintln!( or similar, and then return NS_ERROR_ABORT to continue exiting normally. Main risk of fatalln! is that I'm guessing it panic!s, and could end up looking like a crash on crash-stats.

Ah, so this might have been the reason why it crashes. It's still not clear why I do not see the crash now, but anyway thanks a lot for the hint regarding the behavior of fatalln!(). So I will use your suggested changes to make it a clean exit.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm going to get rid of the Rust component over on bug 1720676. When that gets landed this crash will no longer exist.