Large libnotify alert images trigger a SIGTERM

RESOLVED FIXED in Firefox 59

Status

()

P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: lina, Assigned: agashlin)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

STR:

1. Open https://tests.peter.sh/notification-generator/.
2. Select "Huge icon (PNG, 3333x5230)" from the "Icon" drop-down menu.
3. Click "Display the notification."

The GDB backtrace shows a call to `notify_notification_show`, so we at least make it that far.

Comment 1

a year ago
I can still reproduce it on GNOME version 3.24.2 with Firefox 55.

Test site: https://files.hboeck.de/crashff.html
Duplicate of this bug: 1395940
See some more info in bug 1395940.
Priority: -- → P1
(Assignee)

Comment 4

a year ago
I see that we're always decoding the image to its native size [1].

This seems to be getting stuck in dbus-daemon: when we send a large enough image it rejects it as out of spec (see [2] and [3]), then Firefox halts on the broken connection [4], because glib dbus connections default to exit on close. The notification example is 66.5 MB of RGBA, DBUS_MAXIMUM_ARRAY_LENGTH is 64 MB.

We should probably resize the image if it is going to go over the limit.

It may be a bug for glib to be sending arrays this large as the dbus documentation is pretty clear about the size limit [5].

(At least this what I found through testing on Lubuntu 17.10, I didn't see notify_notification_show explicitly in the backtrace so I may be looking at a different issue?)

[1] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/toolkit/components/alerts/AlertNotification.cpp#250-254
[2] https://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-marshal-validate.c?id=2f8f4d619b16b134671521c2b4aea3a94fb47848#n447
[3] https://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-transport.c?id=2f8f4d619b16b134671521c2b4aea3a94fb47848#n1186
[4] https://github.com/GNOME/glib/blob/glib-2-54/gio/gdbusconnection.c#L802-L807
[5] https://dbus.freedesktop.org/doc/api/html/group__DBusProtocol.html#ga5265afa08a4c8d9f31b287a57e8cb217
(Assignee)

Comment 5

a year ago
Created attachment 8932682 [details] [diff] [review]
Don't attempt to notify with a large icon

Resizing turned out to be more of an ordeal than I expected. In the interest of getting this fixed now, here's a patch that prevents us from sending such large images.

The size limit isn't exactly 64MB because the image data array is wrapped in the hints array, which has the same limit. I added a bit of fudge beyond the 60 bytes that are definitely needed by libnotify. Fortunately we don't set any other hints so there aren't known variable sizes to take into consideration, but parts of the message depend on the protocol.
Assignee: nobody → agashlin
Attachment #8932682 - Flags: review?(karlt)
Comment on attachment 8932682 [details] [diff] [review]
Don't attempt to notify with a large icon

Thank you for the analysis.

If not pushing future patches to reviewboard, then please update ~/.hgrc to
include more context in patches by default.  This might be the relevant
portion:

[diff]
git = true
showfunc = 1
unified = 8
Attachment #8932682 - Flags: review?(karlt) → review+
(Assignee)

Comment 7

a year ago
Thanks karlt, and sorry about the ugly patch, I'll make those changes and try to remember reviewboard next time.

Try looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52016dfa7941711286bbc7c9fbdb5a2b90452fa6
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2007a896005
Don't send a large notify image. r=karlt
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2007a896005
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.