Closed
Bug 1418770
Opened 5 years ago
Closed 4 years ago
Lack of dbus name-space sanitization makes Firefox crash at startup
Categories
(Core Graveyard :: X-remote, defect)
Core Graveyard
X-remote
Tracking
(firefox-esr52 unaffected, firefox58 unaffected, firefox59+ fixed, firefox60+ fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | + | fixed |
firefox60 | + | fixed |
People
(Reporter: kubrick, Assigned: stransky)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0 Build ID: 20171117100127 Steps to reproduce: After the latest update on nightly, firefox crashes on startup report 1ea5f017-cc7a-441a-b236-775b80171119 for example reverting to 2017-11-17 works. Actual results: dbus[17992]: arguments to dbus_bus_request_name() were incorrect, assertion "_dbus_check_is_valid_bus_name (name)" failed in file dbus-bus.c line 1122. This is normally a bug in some application using the D-Bus library. D-Bus not built with -rdynamic so unable to print a backtrace Redirecting call to abort() to mozalloc_abort ExceptionHandler::GenerateDump cloned child 18102 ExceptionHandler::SendContinueSignalToChild sent continue signal to child ExceptionHandler::WaitForContinueSignal waiting for continue signal...
Reporter | ||
Updated•5 years ago
|
Crash Signature: 1ea5f017-cc7a-441a-b236-775b80171119
17c43b6d-23f9-4189-a567-6c9540171119
Reporter | ||
Comment 1•5 years ago
|
||
This is the backtrace I get if I disable google breakpad: #0 0x000000000040896b in mozalloc_abort(char const*) () #1 0x0000000000408935 in mozalloc_abort(char const*) () #2 0x00007fffef1384e7 in _dbus_abort () at /usr/lib/libdbus-1.so.3 #3 0x00007fffef12e9a3 in () at /usr/lib/libdbus-1.so.3 #4 0x00007fffef1107c8 in dbus_bus_request_name () at /usr/lib/libdbus-1.so.3 #5 0x00007fffe93e449d in nsDBusRemoteService::Startup(char const*, char const*) () at /home/francois/bin/firefox-nightly/libxul.so #6 0x00007fffe93e4d9c in nsRemoteService::Startup(char const*, char const*) () at /home/francois/bin/firefox-nightly/libxul.so #7 0x00007fffe94b46ab in XREMain::XRE_mainRun() [clone .cold.89] () at /home/francois/bin/firefox-nightly/libxul.so #8 0x00007fffea46b958 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) () at /home/francois/bin/firefox-nightly/libxul.so #9 0x00007fffea46b5b6 in XRE_main(int, char**, mozilla::BootstrapConfig const&) () at /home/francois/bin/firefox-nightly/libxul.so #10 0x0000000000421bf9 in do_main(int, char**, char**) () #11 0x0000000000415bdf in main ()
Reporter | ||
Comment 2•5 years ago
|
||
This was introduced by https://hg.mozilla.org/mozilla-central/diff/8a07d34a17b6/toolkit/components/remote/nsDBusRemoteService.cpp#l1.177 not checking if the profile name is a valid name for DBUS and crashing if the profile name contains spaces or other characters not in "[A-Z][a-z][0-9]_" as per https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names Also, the total length must be checked not to be over 255 characters long
Summary: abort on startup with nightly post nov 18 (arguments to dbus_bus_request_name() were incorrect) → Lack of dbus name-space sanitization makes Firefox crash at startup
Reporter | ||
Comment 3•5 years ago
|
||
My opinion is that the random string prefix of the profile directory should be used as it is unique (just removing invalid characters would open the door to name collisions) and short.
Updated•5 years ago
|
Comment 4•4 years ago
|
||
hi, is this still an issue after bug 1420124 has landed?
Flags: needinfo?(kubrick)
Reporter | ||
Comment 5•4 years ago
|
||
Yes this is still an issue. If the profile name is long enough to produce a base64 string longer than 256 character, the crash still occurs. You could truncate it, but then name collisions can occur. I maintain that "the random string prefix of the profile directory should be used as it is unique (just removing invalid characters would open the door to name collisions) and short."
Flags: needinfo?(kubrick)
Updated•4 years ago
|
Blocks: 1360566
status-firefox58:
--- → unaffected
status-firefox59:
--- → affected
status-firefox60:
--- → affected
Keywords: regression
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → stransky
Flags: needinfo?(stransky)
Assignee | ||
Comment 7•4 years ago
|
||
I'll look at it.
Assignee | ||
Updated•4 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment hidden (mozreview-request) |
Comment 9•4 years ago
|
||
mozreview-review |
Comment on attachment 8950192 [details] Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH, https://reviewboard.mozilla.org/r/219454/#review225544 r+ with issue fixed. ::: commit-message-84151:1 (Diff revision 1) > +Bug 1418770 - Truncate DBus names to DBUS_MAXIMUM_NAME_LENGTH, r?jhorak maybe little more details there: Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH ::: toolkit/components/remote/nsDBusRemoteService.cpp:116 (Diff revision 1) > (strcmp("org.freedesktop.DBus.Introspectable", iface) == 0)) { > return Introspect(msg); > } > > nsAutoCString ourInterfaceName; > ourInterfaceName = nsPrintfCString("org.mozilla.%s", mAppName.get()); I guess we need to check also this to be extra sure.
Attachment #8950192 -
Flags: review?(jhorak) → review+
Reporter | ||
Comment 10•4 years ago
|
||
What if two long profile names start the same? You'll have a dbus name collision. Why don't you just use the random string prefix of the profile directory? it's there, it's unique, identifiable and dependably short.
Assignee | ||
Comment 11•4 years ago
|
||
(In reply to Francois Guerraz from comment #10) > What if two long profile names start the same? You'll have a dbus name > collision. > > Why don't you just use the random string prefix of the profile directory? > it's there, it's unique, identifiable and dependably short. Unfortunately the random string prefix is not available for remote code. We have only the profile name given as command line argument (-P profile_name). We even don't have default profile name which is solved at Bug 1434544.
Assignee | ||
Comment 12•4 years ago
|
||
(In reply to Martin Stránský [:stransky] from comment #11) > (In reply to Francois Guerraz from comment #10) > > What if two long profile names start the same? You'll have a dbus name > > collision. > > > > Why don't you just use the random string prefix of the profile directory? > > it's there, it's unique, identifiable and dependably short. > > Unfortunately the random string prefix is not available for remote code. We > have only the profile name given as command line argument (-P profile_name). > We even don't have default profile name which is solved at Bug 1434544. Err, we actually have the default profile name but for remote service only (not for remote client).
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Component: General → X-remote
Product: Toolkit → Core
Jimm, can you help get this landed and request uplift?
tracking-firefox59:
--- → +
tracking-firefox60:
--- → +
Flags: needinfo?(jmathies)
Keywords: checkin-needed
Assignee | ||
Comment 15•4 years ago
|
||
Comment on attachment 8950192 [details] Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH, Approval Request Comment [Feature/Bug causing the regression]: Bug 1360566 [User impact if declined]: Firefox can't start with very long profile name (longer than ~230 chars) [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: no one [Is the change risky?]: no [Why is the change risky/not risky?]: It only truncates profile name which is passed to DBus interface. The truncated name is not used by Firefox itself. [String changes made/needed]: none
Attachment #8950192 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 16•4 years ago
|
||
Hm, looks like I forgot to land the commit. Landed now.
Comment 17•4 years ago
|
||
Pushed by stransky@redhat.com: https://hg.mozilla.org/integration/autoland/rev/44ad70613f51 Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH, r=jhorak
Keywords: checkin-needed
Comment 18•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44ad70613f51
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 19•4 years ago
|
||
Comment on attachment 8950192 [details] Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH, Taking for 59b12.
Flags: needinfo?(jmathies)
Attachment #8950192 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
![]() |
||
Comment 20•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/28b5b075c5e6
Reporter | ||
Comment 21•4 years ago
|
||
I have 3 profiles, *default *aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc *aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccd I launched all three of them, but as predicted, there's a dbus name collision $ dbus-send --session --dest=org.freedesktop.DBus --type=method_call --print-reply /org/freedesktop/DBus org.freedesktop.DBus.ListNames | grep firefox string "org.mozilla.firefox.YWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJjY2NjY2NjY2N" string "org.mozilla.firefox.ZGVmYXVsdA__" I do not know what are the consequences of that but it can't be good.
Flags: needinfo?(stransky)
Assignee | ||
Comment 22•4 years ago
|
||
(In reply to Francois Guerraz from comment #21) > I have 3 profiles, > > *default > *aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa > aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbb > bbbbbbbbbbbbbbcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc > *aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa > aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbb > bbbbbbbbbbbbbbcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccd > > I launched all three of them, but as predicted, there's a dbus name collision > $ dbus-send --session --dest=org.freedesktop.DBus > --type=method_call --print-reply > /org/freedesktop/DBus org.freedesktop.DBus.ListNames | grep firefox > string > "org.mozilla.firefox. > YWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhY > WFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYW > FhYWFhYWFhYWJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJjY2NjY2N > jY2N" > string "org.mozilla.firefox.ZGVmYXVsdA__" > > I do not know what are the consequences of that but it can't be good. Consequence is that if you have already running a Firefox with profile name longer that ~230 chars and you start another firefox instance with different profile name longer than ~230 chars the DBus remote service fails to start for the second one.
Flags: needinfo?(stransky)
Updated•4 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 23•4 years ago
|
||
This appears to have broken snaps if I understand correctly.
Depends on: 1441894
Comment 24•4 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #23) > This appears to have broken snaps if I understand correctly. I don't think this change actually broke the snap, but snap isn't allowing it to bind to those dbus names. This would have been an issue before as well.
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•