After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 781034 - monitor-manager-kms may set incorrect libdrm event context version
monitor-manager-kms may set incorrect libdrm event context version
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
git master
Other Linux
: Normal major
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2017-04-07 13:32 UTC by Daniel Stone
Modified: 2019-01-02 14:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
monitor-manager-kms: Use correct DRM event context version (1.47 KB, patch)
2017-04-07 13:33 UTC, Daniel Stone
committed Details | Review

Description Daniel Stone 2017-04-07 13:32:56 UTC
DRM_EVENT_CONTEXT_VERSION is the latest context version supported by
whatever version of libdrm is present. Mutter was blindly asserting it
supported whatever version that may be, even if it actually didn't.

With libdrm 2.4.78, setting a higher context version than 2 will attempt
to call the page_flip_handler2 vfunc if it was non-NULL, which being a
random chunk of stack memory, it might well have been.

Set the version as 2, which should be bumped only with the appropriate
version checks.
Comment 1 Daniel Stone 2017-04-07 13:33:11 UTC
Created attachment 349476 [details] [review]
monitor-manager-kms: Use correct DRM event context version

DRM_EVENT_CONTEXT_VERSION is the latest context version supported by
whatever version of libdrm is present. Mutter was blindly asserting it
supported whatever version that may be, even if it actually didn't.

With libdrm 2.4.78, setting a higher context version than 2 will attempt
to call the page_flip_handler2 vfunc if it was non-NULL, which being a
random chunk of stack memory, it might well have been.

Set the version as 2, which should be bumped only with the appropriate
version checks.
Comment 2 Daniel Stone 2017-04-07 13:33:36 UTC
Please merge when you can; I don't have commit access. This is also a good candidate for stable branches.
Comment 3 Giovanni Campagna 2017-04-08 12:56:46 UTC
Can I say I don't understand why this patch is needed?

If mutter is compiled against old libdrm and run against old libdrm, the version will be the old one and libdrm will not touch page_flip_handler2.

If mutter is compiled against old libdrm, and run against new libdrm, the version should still be the old one (it's a constant), so the new libdrm should not look at page_flip_handler2 (because it will be garbage).

If mutter is compiled against new libdrm, the version will be new but at the same time the struct will be one pointer larger, meaning that the memset will zero out the pointer and libdrm should not call page_flip_handler2.

When is this causing issues?
Comment 4 Jonas Ådahl 2017-04-10 06:34:21 UTC
I'd argue the patch is correct, but it indeed seems more like a cleanup than anything needed, given Giovannis reasoning above.
Comment 5 Daniel Stone 2017-04-10 06:41:50 UTC
You're right, in that the memset helps. But equally, a context version bump could change semantics and behaviour entirely, and you'd end up being broken by it.
Comment 6 Daniel Stone 2017-07-25 11:04:13 UTC
FWIW, the patch does still apply.
Comment 7 Jonas Ådahl 2019-01-02 14:47:15 UTC
This wasn't landed for some reason. I reworked the patch (the affected function had moved to another file), and push.d