1420124 - Check profile name before use them at DBus strings

Status: RESOLVED FIXED

(Core Graveyard :: X-remote, defect, P1)

Description

[Martin Stránský [:stransky] (ni? me)]

+++ This bug was initially created as a clone of Bug #1418985 +++

Follow up from Bug 1418985 comment 11. Wrong profile name crashes DBus interface:

#0  0x000000000040898b in mozalloc_abort(char const*) ()
#1  0x0000000000408955 in mozalloc_abort(char const*) ()
#2  0x00007fffeebd88c4 in _dbus_abort () at ../../dbus/dbus-sysdeps.c:93
#3  0x00007fffeebcf150 in _dbus_warn_check_failed (format=format@entry=0x7fffeebdee68 "arguments to %s() were incorrect, assertion \"%s\" failed in file %s line %d.\nThis is normally a bug in some application using the D-Bus library.\n")
    at ../../dbus/dbus-internals.c:281
#4  0x00007fffeebcf86a in _dbus_warn_return_if_fail (function=function@entry=0x7fffeebd9be0 <__func__.4674> "dbus_bus_request_name", assertion=assertion@entry=0x7fffeebd99c0 "_dbus_check_is_valid_bus_name (name)", file=file@entry=0x7fffeebd99e5 "../../dbus/dbus-bus.c", line=line@entry=1122) at ../../dbus/dbus-internals.c:936
#5  0x00007fffeebb1c07 in dbus_bus_request_name (connection=0x7ffff685d010, name=<optimized out>, flags=<optimized out>, error=0x7fffffffc830) at ../../dbus/dbus-bus.c:1122
#6  0x00007fffe8e7bae6 in nsDBusRemoteService::Startup(char const*, char const*) ()
    at /home/komat/Programy/firefox-nightly/libxul.so
#7  0x00007fffe8e7c3fa in nsRemoteService::Startup(char const*, char const*) ()
    at /home/komat/Programy/firefox-nightly/libxul.so
#8  0x00007fffe8f4bef5 in XREMain::XRE_mainRun() [clone .cold.89] () at /home/komat/Programy/firefox-nightly/libxul.so
#9  0x00007fffe9efbfd8 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) ()
    at /home/komat/Programy/firefox-nightly/libxul.so
#10 0x00007fffe9efbc36 in XRE_main(int, char**, mozilla::BootstrapConfig const&) ()
    at /home/komat/Programy/firefox-nightly/libxul.so
#11 0x0000000000421e39 in do_main(int, char**, char**) ()
#12 0x0000000000415e8f in main ()

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1420124 - X Remote Service: Encode DBus interface strings by base64 to avoid failure/crashes,

Check DBus arguments by "dbus_validate_*" routines before pass them to DBus interface. If the name is invalid try to create a fallback and eventually give up to avoid crashes (Bug 1418985).

Also remove DBusError where it's not useful.

Review commit: https://reviewboard.mozilla.org/r/202452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202452/

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1420124 - X Remote client: Encode DBus interface strings by base64 to avoid failure/crashes,

Check DBus arguments by "dbus_validate_*" routines before pass them to DBus interface. If the name is invalid try to create a
fallback and eventually give up to avoid crashes (Bug 1418985).

Review commit: https://reviewboard.mozilla.org/r/202454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202454/

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Hm, looks like the dbus_validate_*() routines was added at D-Bus 1.5.12 so we'd need to dlsym them.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8931304 [details]
Bug 1420124 - X Remote Service: Encode DBus interface strings by base64 to avoid failure/crashes,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202452/diff/1-2/

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8931305 [details]
Bug 1420124 - X Remote client: Encode DBus interface strings by base64 to avoid failure/crashes,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202454/diff/1-2/

Comment 7

[Jan Horak [:jhorak]]

Comment on attachment 8931304 [details]
Bug 1420124 - X Remote Service: Encode DBus interface strings by base64 to avoid failure/crashes,

https://reviewboard.mozilla.org/r/202452/#review208882

Using base64 looks good.

Comment 8

[Jan Horak [:jhorak]]

Comment on attachment 8931305 [details]
Bug 1420124 - X Remote client: Encode DBus interface strings by base64 to avoid failure/crashes,

https://reviewboard.mozilla.org/r/202454/#review208886

::: widget/xremoteclient/DBusRemoteClient.cpp:96
(Diff revision 2)
> +  nsAutoCString profileName;
> +  nsresult rv = mozilla::Base64Encode(nsAutoCString(aProfileName), profileName);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  profileName.ReplaceChar("+/=", '_');
>  
>    nsAutoCString destinationName;

Probably leftover of renaming to busName. Please consider if the rename makes things more clear according to
https://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html#ga98ddc82450d818138ef326a284201ee0

::: widget/xremoteclient/DBusRemoteClient.cpp:97
(Diff revision 2)
> +  nsresult rv = mozilla::Base64Encode(nsAutoCString(aProfileName), profileName);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  profileName.ReplaceChar("+/=", '_');
>  
>    nsAutoCString destinationName;
> -  destinationName = nsPrintfCString("org.mozilla.%s.%s", aProgram, aProfile);
> +  busName = nsPrintfCString("org.mozilla.%s.%s", aProgram, profileName.get());

The busName is not declared there.

Comment 9

[skierpage]

Thanks for fixing this! I think the patch turns "My test Profile$" into org.mozilla.TXkgdGVzdCBQcm9maWxlJA__ ; if you only encoded the invalid characters, producing org.mozilla.MyIA__testIA__ProfileJA__ , the bus name would be more recognizable in errors and D-Bus debugging. Easier said than done?

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8931305 [details]
Bug 1420124 - X Remote client: Encode DBus interface strings by base64 to avoid failure/crashes,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202454/diff/2-3/

Comment 11

[Martin Stránský [:stransky] (ni? me)]

(In reply to skierpage from comment #9)
> Thanks for fixing this! I think the patch turns "My test Profile$" into
> org.mozilla.TXkgdGVzdCBQcm9maWxlJA__ ; if you only encoded the invalid
> characters, producing org.mozilla.MyIA__testIA__ProfileJA__ , the bus name
> would be more recognizable in errors and D-Bus debugging. Easier said than
> done?

That makes sense but let's fix the crashes first and then we can adjust the dbus name. Btw. it should be "org.mozilla.firefox.TXkgdGVzdCBQcm9maWxlJA__" name as the application name is also used.

Comment 12

[Jan Horak [:jhorak]]

Comment on attachment 8931305 [details]
Bug 1420124 - X Remote client: Encode DBus interface strings by base64 to avoid failure/crashes,

https://reviewboard.mozilla.org/r/202454/#review209718

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d971e9a4650d
X Remote Service: Encode DBus interface strings by base64 to avoid failure/crashes, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/d338012d4d45
X Remote client: Encode DBus interface strings by base64 to avoid failure/crashes, r=jhorak

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d971e9a4650d
https://hg.mozilla.org/mozilla-central/rev/d338012d4d45

Comment 15

[Clemens Eisserer]

is it really fixed:

I've just experienced crashes bp-580c1d3f-4ed6-4920-9d0a-e4dfe0180105 and bp-47484bd3-5a3c-41fd-b4e1-bb9f80180105 and the crash reporter page linked the crash to this report.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

(In reply to Clemens Eisserer from comment #15)
> is it really fixed:
> 
> I've just experienced crashes bp-580c1d3f-4ed6-4920-9d0a-e4dfe0180105 and
> bp-47484bd3-5a3c-41fd-b4e1-bb9f80180105 and the crash reporter page linked
> the crash to this report.

Crashed fixed at this bug report causes Firefox does not start with particular profile - is that your case? If so can you please post your profile name? (you can find it at about:profiles - I'm interested in the path like ".mozilla/firefox/80thyv9a.default").

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Same for me on Arch Linux. My profile path is ~/.mozilla/firefox/cp2auh9b.default.

https://crash-stats.mozilla.com/report/index/c3283d4c-0303-453b-a88d-8fe800180108

Comment 18

[Martin Stránský [:stransky] (ni? me)]

(In reply to Tim Taubert [:ttaubert] from comment #17)
> Same for me on Arch Linux. My profile path is
> ~/.mozilla/firefox/cp2auh9b.default.
> 
> https://crash-stats.mozilla.com/report/index/c3283d4c-0303-453b-a88d-
> 8fe800180108

Thanks. Can you reliably reproduce the crash? If so can you please attach a full backtrace from gdb? You need to install dbus debug symbols for that.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Here's a backtrace. Reproduces every time I provide a -profile argument.

#0  0x00000000004082d8 in mozalloc_abort(char const*) ()
#1  0x00000000004082a2 in mozalloc_abort(char const*) ()
#2  0x00007fffef1374d7 in _dbus_abort () at dbus-sysdeps.c:93
#3  0x00007fffef12d9a3 in _dbus_warn_check_failed (format=format@entry=0x7fffef13da28 "arguments to %s() were incorrect, assertion \"%s\" failed in file %s line %d.\nThis is normally a bug in some application using the D-Bus library.\n") at dbus-internals.c:281
#4  0x00007fffef12e0db in _dbus_warn_return_if_fail (function=function@entry=0x7fffef138820 <__func__.4674> "dbus_bus_request_name", assertion=assertion@entry=0x7fffef138600 "_dbus_check_is_valid_bus_name (name)", file=file@entry=0x7fffef138625 "dbus-bus.c", line=line@entry=1122) at dbus-internals.c:936
#5  0x00007fffef10f7c8 in dbus_bus_request_name (connection=0x7fffe3536890, name=<optimized out>, flags=<optimized out>, error=0x7fffffffce00) at dbus-bus.c:1122
#6  0x00007fffe93516a6 in nsDBusRemoteService::Startup(char const*, char const*) () at /home/tim/bin/firefox-nightly/libxul.so
#7  0x00007fffe9351faa in nsRemoteService::Startup(char const*, char const*) () at /home/tim/bin/firefox-nightly/libxul.so
#8  0x00007fffe942db57 in XREMain::XRE_mainRun() [clone .cold.90] () at /home/tim/bin/firefox-nightly/libxul.so
#9  0x00007fffea40a878 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) () at /home/tim/bin/firefox-nightly/libxul.so
#10 0x00007fffea40a4d6 in XRE_main(int, char**, mozilla::BootstrapConfig const&) () at /home/tim/bin/firefox-nightly/libxul.so
#11 0x0000000000420d99 in do_main(int, char**, char**) ()
#12 0x000000000041536f in main ()

Comment 20

[Martin Stránský [:stransky] (ni? me)]

(In reply to Tim Taubert [:ttaubert] from comment #19)
> Here's a backtrace. Reproduces every time I provide a -profile argument.
> 
> #0  0x00000000004082d8 in mozalloc_abort(char const*) ()
> #1  0x00000000004082a2 in mozalloc_abort(char const*) ()
> #2  0x00007fffef1374d7 in _dbus_abort () at dbus-sysdeps.c:93
> #3  0x00007fffef12d9a3 in _dbus_warn_check_failed
> (format=format@entry=0x7fffef13da28 "arguments to %s() were incorrect,
> assertion \"%s\" failed in file %s line %d.\nThis is normally a bug in some
> application using the D-Bus library.\n") at dbus-internals.c:281
> #4  0x00007fffef12e0db in _dbus_warn_return_if_fail
> (function=function@entry=0x7fffef138820 <__func__.4674>
> "dbus_bus_request_name", assertion=assertion@entry=0x7fffef138600
> "_dbus_check_is_valid_bus_name (name)", file=file@entry=0x7fffef138625
> "dbus-bus.c", line=line@entry=1122) at dbus-internals.c:936
> #5  0x00007fffef10f7c8 in dbus_bus_request_name (connection=0x7fffe3536890,
> name=<optimized out>, flags=<optimized out>, error=0x7fffffffce00) at
> dbus-bus.c:1122
> #6  0x00007fffe93516a6 in nsDBusRemoteService::Startup(char const*, char
> const*) () at /home/tim/bin/firefox-nightly/libxul.so
> #7  0x00007fffe9351faa in nsRemoteService::Startup(char const*, char const*)
> () at /home/tim/bin/firefox-nightly/libxul.so

Great, Thanks. Can you also provide actual profile name you used as an argument, params of
nsDBusRemoteService::Startup() (this is the profile name) and also the name from _dbus_check_is_valid_bus_name() if that's possible. You may need to install firefox debuginfo package for that.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Martin Stránský [:stransky] from comment #20)
> Great, Thanks. Can you also provide actual profile name you used as an
> argument, params of
> nsDBusRemoteService::Startup() (this is the profile name) and also the name
> from _dbus_check_is_valid_bus_name() if that's possible. You may need to
> install firefox debuginfo package for that.

I launched Firefox/GDB with:

> gdb --args ~/bin/firefox-nightly/firefox -profile ~/.mozilla/firefox/cp2auh9b.default

aProfileName seems empty...

> #6  0x00007fffe69eb199 in nsDBusRemoteService::Startup(char const*, char const*) (this=0x7fffc7cbf940, aAppName=0x7ffff6b89120 "firefox", aProfileName=0x7fffffffd1ac "") at /home/tim/workspace/gecko-dev/toolkit/components/remote/nsDBusRemoteService.cpp:185
> #7  0x00007fffe69ec607 in nsRemoteService::Startup(char const*, char const*) (this=0x7fffc7cbf0d0, aAppName=0x7ffff6b89120 "firefox", aProfileName=0x7fffffffd1ac "")
>     at /home/tim/workspace/gecko-dev/toolkit/components/remote/nsRemoteService.cpp:40

So we end up with `busName` = "org.mozilla.firefox." and the validate function rejects trailing dots:

https://github.com/kobolabs/dbus/blob/566df92e5319a50e0a6223afccee8f1b9a03b8d5/dbus/dbus-marshal-validate.c#L1071

Comment 22

[Martin Stránský [:stransky] (ni? me)]

(In reply to Tim Taubert [:ttaubert] from comment #21)
> (In reply to Martin Stránský [:stransky] from comment #20)
> > Great, Thanks. Can you also provide actual profile name you used as an
> > argument, params of
> > nsDBusRemoteService::Startup() (this is the profile name) and also the name
> > from _dbus_check_is_valid_bus_name() if that's possible. You may need to
> > install firefox debuginfo package for that.
> 
> I launched Firefox/GDB with:
> 
> > gdb --args ~/bin/firefox-nightly/firefox -profile ~/.mozilla/firefox/cp2auh9b.default
> 
> aProfileName seems empty...
> 
> > #6  0x00007fffe69eb199 in nsDBusRemoteService::Startup(char const*, char const*) (this=0x7fffc7cbf940, aAppName=0x7ffff6b89120 "firefox", aProfileName=0x7fffffffd1ac "") at /home/tim/workspace/gecko-dev/toolkit/components/remote/nsDBusRemoteService.cpp:185
> > #7  0x00007fffe69ec607 in nsRemoteService::Startup(char const*, char const*) (this=0x7fffc7cbf0d0, aAppName=0x7ffff6b89120 "firefox", aProfileName=0x7fffffffd1ac "")
> >     at /home/tim/workspace/gecko-dev/toolkit/components/remote/nsRemoteService.cpp:40
> 
> So we end up with `busName` = "org.mozilla.firefox." and the validate
> function rejects trailing dots:
> 
> https://github.com/kobolabs/dbus/blob/
> 566df92e5319a50e0a6223afccee8f1b9a03b8d5/dbus/dbus-marshal-validate.c#L1071

Filed as Bug 1429021, Thanks.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Thanks!