Don't crash if a client (legally) uses a deleted global
ClosedPublic

Authored by davidedmundson on Sep 18 2017, 4:26 PM.

Details

Summary

There is a race condition in the following situation:

  • Server creates a global
  • Client binds to that global (making a new resource for that global)

Simultaneously:

  • The client uses this resource
  • The server deletes the global

We then process an event for a resource linked to a deleted global.

This is noted in the specification, the client documentation says:
"The object remains valid and requests to the object will be
ignored until the client destroys it, to avoid races between the global
going away and a client sending a request to it. "

KWayland does not handle this at all.

The global's user data refer to our C++ wrapper
The resource's user data refer to *the same* C++ wrapper

When the global is deleted the resource user data now refers to garbage.

To fix the issue, instead of setting the resource userdata to the
global, we set it to a smartpointer to the global stored on the heap.

We can then validate if our global is still valid.

Theoretically this applies to every global

Practically there are only 3 globals that don't have the lifespan of the
server. Output (which is read only and doesn't matter), Blur and
BackgroundContrast.

Blur resets it's global when a screen geometry changes.

Unfotunately this exactly at the same time that Plasmashell is
doing a lot of processing and creating some blurs.

Test Plan

See unit test

Diff Detail

Repository
R127 KWayland
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson created this revision.Sep 18 2017, 4:26 PM
Restricted Application added projects: Plasma on Wayland, Frameworks. · View Herald TranscriptSep 18 2017, 4:26 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript

Mem leak in test

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptSep 18 2017, 4:37 PM
anthonyfieroni added inline comments.
src/server/blur_interface.cpp
49–53

QPointer itself can determine that resource is deleted but QPointer* acts like every normal pointer e.g. it can be dangling. So if unbind is called before cast this line will crash.

89

To work your idea this line should be

delete reinterpret_cast<QPointer<BlurManagerInterface>*>(wl_resource_get_user_data(r))->data()

QPointer *should* live to not be dangling in cast, so you can have a static map resource to QPointer* then you can update and delete them at server die.

davidedmundson marked 2 inline comments as done.Sep 18 2017, 6:39 PM
davidedmundson added inline comments.
src/server/blur_interface.cpp
49–53

I'm not going to get a callback on a resource if the resource doesn't exist. (Libwayland would have already crashed if that happens)

That's not what we're guarding against here.

graesslin accepted this revision.Sep 18 2017, 6:45 PM
graesslin added a subscriber: graesslin.

What surprises me is that BlurManager recreates for Output changes. That sounds like a bug in KWin (or an area which could be improved).

This revision is now accepted and ready to land.Sep 18 2017, 6:45 PM
anthonyfieroni added inline comments.Sep 18 2017, 6:58 PM
src/server/blur_interface.cpp
49–53

I don't get that. So your patch is quite enough, treat this note as a noise :)

graesslin added inline comments.Sep 18 2017, 7:13 PM
src/server/blur_interface.cpp
49–53

The cast method here is only called from callbacks. Wayland works by having a global callback table where one just gets a wl_resource pointer which contains a reference to a user data. As long as the client doesn't call unbind on the wl_resource the pointer cannot dangle. That's why it won't happen here.

This revision was automatically updated to reflect the committed changes.
davidedmundson marked an inline comment as done.