Bug 462308 - MediaProxy::determineBackgroundType needlessly instantiates all available QImage reader plugins which can cause Plasma to crash on launch in a loop
Summary: MediaProxy::determineBackgroundType needlessly instantiates all available QIm...
Status: RESOLVED FIXED
Alias: None
Product: plasmashell
Classification: Plasma
Component: Image & Slideshow wallpaper plugins (show other bugs)
Version: master
Platform: Other Linux
: HI normal
Target Milestone: 1.0
Assignee: Plasma Bugs List
URL: https://bugzilla.redhat.com/show_bug....
Keywords: regression
Depends on:
Blocks:
 
Reported: 2022-11-27 12:49 UTC by Kevin Kofler
Modified: 2022-11-28 05:04 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.26.4
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Kofler 2022-11-27 12:49:53 UTC
SUMMARY
In order to determine whether the wallpaper is animated or not, MediaProxy::determineBackgroundType uses the following algorithm:
if (QMovie::supportedFormats().contains(QFileInfo(filePath).suffix().toLower().toLatin1())) {
This enumerates all image types that support animations, then checks whether the one we care about is contained in the list.

Unfortunately, to check which image types support animations, QMovie::supportedFormats() needs to instantiate each and every image reader plugin that is available and ask it whether it supports animations.

STEPS TO REPRODUCE
We have caught this in Fedora by accident, because we ran into a bug:
https://bugzilla.redhat.com/show_bug.cgi?id=2144200
where Chromium and PDFium would fail to load on Apple hardware using "Apple Silicon" ARM CPUs. This surprisingly lead to the whole Plasma shell failing to start because the QImage PDF reader would crash during initialization, crashing the entire shell along with it. Since image reader plugins can generally be of varying quality, it would be much safer to avoid instantiating less tested exotic plugins when we do not actually need them.

OBSERVED RESULT
A crash in PDFium can crash the entire Plasma Shell, turning QtPdf into a critical package.

EXPECTED RESULT
The Plasma Shell should not load PDFium unless the wallpaper is actually a PDF. And likewise for other rarely used image decoding plugins.

SOFTWARE/OS VERSIONS
All current Plasma versions, on all hardware. The crash can be reproduced by using an unfixed QtWebEngine/QtPdf on Apple Silicon hardware, but the underlying issue exists everywhere.

ADDITIONAL INFORMATION
Instead of enumerating all image formats that support animation using QMovie::supportedFormats():

    if (QMovie::supportedFormats().contains(QFileInfo(filePath).suffix().toLower().toLatin1())) {

the better way would be to do what QMovie::supportedFormats() does internally, but only on the type we actually care about, without enumerating them:

    QBuffer dummyBuffer;
    dummyBuffer.open(QIODevice::ReadOnly);

    if (QImageReader(&dummyBuffer, QFileInfo(filePath).suffix().toLower().toLatin1()).supportsOption(QImageIOHandler::Animation)) {

See also the downstream bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=2144200#c22
Comment 1 Bug Janitor Service 2022-11-27 13:12:06 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/plasma-workspace/-/merge_requests/2374
Comment 2 Fushan Wen 2022-11-28 00:34:57 UTC
Git commit 2f743037d132a4cf611e20604032ec7002c99b8b by Fushan Wen.
Committed on 28/11/2022 at 00:22.
Pushed by fusionfuture into branch 'master'.

Do not query all available image plugins when determining background type

In order to determine whether the wallpaper is animated or not,
MediaProxy::determineBackgroundType uses the following algorithm:

if (QMovie::supportedFormats().contains(QFileInfo(filePath).suffix().toLower().toLatin1())) {

This enumerates all image types that support animations, then
checks whether the one we care about is contained in the list.

Unfortunately, to check which image types support animations,
QMovie::supportedFormats() needs to instantiate each and every image
reader plugin that is available and ask it whether it supports
animations.

STEPS TO REPRODUCE

We have caught this in Fedora by accident, because we ran into a bug:
https://bugzilla.redhat.com/show_bug.cgi?id=2144200
where Chromium and PDFium would fail to load on Apple hardware using
"Apple Silicon" ARM CPUs. This surprisingly lead to the whole Plasma
shell failing to start because the QImage PDF reader would crash
during initialization, crashing the entire shell along with it.
Since image reader plugins can generally be of varying quality, it
would be much safer to avoid instantiating less tested exotic plugins
when we do not actually need them.

A crash in PDFium can crash the entire Plasma Shell, turning QtPdf
into a critical package.

Instead of enumerating all image formats that support animation using
QMovie::supportedFormats(), the better way would be to do what
QMovie::supportedFormats() does internally, but only on the type we
actually care about, without enumerating them.

See also the downstream bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=2144200#c22

Co-authored-by: Kevin Kofler <kevin.kofler@chello.at>
FIXED-IN: 5.26.4

M  +7    -1    wallpapers/image/plugin/utils/mediaproxy.cpp

https://invent.kde.org/plasma/plasma-workspace/commit/2f743037d132a4cf611e20604032ec7002c99b8b
Comment 3 Fushan Wen 2022-11-28 02:34:09 UTC
Git commit fe738f146f7a1ffafb4d242ac130760b97f2ef32 by Fushan Wen.
Committed on 28/11/2022 at 02:34.
Pushed by fusionfuture into branch 'cherry-pick-2f743037'.

Do not query all available image plugins when determining background type

In order to determine whether the wallpaper is animated or not,
MediaProxy::determineBackgroundType uses the following algorithm:

if (QMovie::supportedFormats().contains(QFileInfo(filePath).suffix().toLower().toLatin1())) {

This enumerates all image types that support animations, then
checks whether the one we care about is contained in the list.

Unfortunately, to check which image types support animations,
QMovie::supportedFormats() needs to instantiate each and every image
reader plugin that is available and ask it whether it supports
animations.

STEPS TO REPRODUCE

We have caught this in Fedora by accident, because we ran into a bug:
https://bugzilla.redhat.com/show_bug.cgi?id=2144200
where Chromium and PDFium would fail to load on Apple hardware using
"Apple Silicon" ARM CPUs. This surprisingly lead to the whole Plasma
shell failing to start because the QImage PDF reader would crash
during initialization, crashing the entire shell along with it.
Since image reader plugins can generally be of varying quality, it
would be much safer to avoid instantiating less tested exotic plugins
when we do not actually need them.

A crash in PDFium can crash the entire Plasma Shell, turning QtPdf
into a critical package.

Instead of enumerating all image formats that support animation using
QMovie::supportedFormats(), the better way would be to do what
QMovie::supportedFormats() does internally, but only on the type we
actually care about, without enumerating them.

See also the downstream bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=2144200#c22

Co-authored-by: Kevin Kofler <kevin.kofler@chello.at>
FIXED-IN: 5.26.4


(cherry picked from commit 2f743037d132a4cf611e20604032ec7002c99b8b)

M  +7    -1    wallpapers/image/plugin/utils/mediaproxy.cpp

https://invent.kde.org/plasma/plasma-workspace/commit/fe738f146f7a1ffafb4d242ac130760b97f2ef32