Fix covering of composited notifications

[sandsmark]

Use the XComposite extension to get the composite overlay window,
instead of just using the normal root window. This ensures that
composited windows are covered.

Fixes issue #22

[sandsmark]

I guess it should be squashed before merging...

[Airblader]

Yes, but first we should discuss whether it will be merged at all. The i3 suite generally doesn't support compositing and doesn't depend on compositing. On the other hand it's definitely a security related change.

How does this behave if the X server is compiled without the Composite extension?

[sandsmark]

the call should return an error which the code handles by just reverting to the old behavior, but I haven't tested it.

[stapelberg]

Please let us know once you’ve tested it and I’ll have a look at this PR.

[sandsmark]

I'll try to find time to do that.

[sandsmark]

Tested with a xorg-server built with --disable-composite.

[sandsmark]

I also changed the Makefile to make it build with libxcb built with --disable-composite.

[stapelberg]

Thanks for testing.

I also changed the Makefile to make it build with libxcb built with --disable-composite.

Is that a common situation or do major distributions ship with libxcb-composite? In general, we don’t use optional dependencies in i3*.

[sandsmark]

I don't know of any distributions that doesn't ship with libxcb-composite. Even Slackware (the most conservative distro I could think of right now) ships it in their xcb package.

I did it optionally because of Airblader's comment about not depending on compositing.

[stapelberg]

I did it optionally because of Airblader's comment about not depending on compositing.

That comment meant (or at least I think it should have meant) depending on compositing being enabled at runtime.

[Airblader]

Yes, what I had in mind was VNC as we had problems with extensions with those before.

[sandsmark]

FWIW, xcb_composite_release_overlay_window() is not necessary, the overlay window is released when the X11 connection is terminated; https://www.x.org/archive/X11R7.5/doc/compositeproto/compositeproto.txt

[stapelberg]

The changes in your pull request look good, but I’m wondering whether we’ve covered everything we need to cover. It seems like the basic change here is that when a compositor is used, the i3lock window no longer should be a child of the root window, but rather a child of the compositor window.

Does that mean we also need to query the compositor window’s geometry in handle_screen_resize()? (google/xsecurelock@ee1825a seems to do that via the ConfigureNotify event).

Do we need to set the XCB_EVENT_MASK_STRUCTURE_NOTIFY event mask on the compositor window instead of the root window? (line 963)

Do we need to create background pixmaps on the compositor window instead of the root window? (create_bg_pixmap())

And lastly, do we need grab pointer and keyboard on the compositor window instead of the root window? (grab_pointer_and_keyboard())

[karulont]

@stapelberg 's questions are valid.
I did some stuff with composite extension for i3lock-blur. From what I remember the overlay window is special in some ways. I can't answers those questions anymore, but they are rights questions. I also know, that i3lock-blur has problems with composers like compton, thus I think that i3lock-blur can also benefit from those answers.

[sandsmark]

To my understanding; no to all the questions. The composite overlay window is a very special "window". Please see section 3.2 in the document I referred to above.

I have also tested this patch with e. g. launching a compositor after launching i3lock and then launching clients that also try to grab various stuff.

As for xsecurelock, it does several weird things like e. g. releasing the composite window manually.

[stapelberg]

Before merging this, I’ll need to read up on how precisely a compositor works (or can work, as per the spec), and what assumptions compositors might make. I’ll update this PR when I get around to that.

[rickybrent]

If it gets merged in or not, the patch works great for me, thanks. :)

[Airblader]

Can we please use NULL for the pointers? That way it's consistent with the rest of the codebase.

[sandsmark]

fixed

[Airblader]

LGTM. Up to @stapelberg now :-)

[stapelberg]

There isn’t much prior art here — aside from xsecurelock, no other screen locker seems to consider the composite extension.

I’ll go ahead and merge this code, but of course reserve the right to revert it later in case it causes issues or security vulnerabilities.

[divVerent]

To add something to this issue - it seems like it's outright impossible for a screen locker to work with all compositors. For compton there are option combinations where this works (e.g. the default flags, or --backend glx --paint-on-overlay), for cinnamon's compositor it just works - but the spec correctly states that the result of two compositors interacting depends a lot on how the compositor works, and I'm considering making the feature either opt-in or only activate if there is no compositor (or maybe a whitelisted one) in xsecurelock.