sway: Crash in handle_keyboard_repeat

Today I had some time to look into this, and think I’ve mostly tracked it down. Short version: to work around this issue until it’s fixed, add seat * keyboard_grouping none to Sway’s config.

Long version:

On reload, Sway iterates over all devices, and for every seat, resets the device. Resetting a keyboard device calls sway_keyboard_disarm_key_repeat, which works as expected, and does clear the repeat binding.

I added a few pointer prints into Sway which I think show the issue.

The very first “keyboard” to come online is my “Power Button” device.

00:00:00.645 [sway/input/keyboard.c:588] created keyboard 0x61a000048c80                                                                                 
00:00:00.645 [sway/input/keyboard.c:841] Adding keyboard 0:1:Power_Button to group 0x614000070040 

It’s added to a new keyboard group 0x614000070040. A while later, my actual keyboard is added:

00:00:00.778 [sway/input/keyboard.c:588] created keyboard 0x61a00006de80                                                                                 
00:00:00.786 [sway/input/keyboard.c:793] Adding keyboard 10730:258:Kinesis_Advantage2_Keyboard to group 0x614000070040

Oddly, it’s added to the same keyboard group as the power button, under the default keyboard_grouping smart policy. (Minor note here: the definitions of keymaps_match in sway/input/keyboard.c and wlroots/types/wlr_keyboard_group.c do not match; the latter has a bugfix in https://github.com/swaywm/wlroots/commit/b1a63bcd84adad5bffa8ba73dbf64e05c8ce9bc9 that the former does not. This isn’t the issue here at any rate, since adding it does not resolve the crashes.)

Another interesting observation is that whenever I press any key on my keyboard, it is handled twice. For instance, pressing “a”:

00:00:01.427 [sway/input/keyboard.c:499] keyboard GROUP event                                                                                            
00:00:01.427 [sway/input/keyboard.c:341] keyboard device 0x61a000048c80 sent event, key=30, down=1                      
00:00:01.427 [sway/input/keyboard.c:491] keyboard event                                                                 
00:00:01.427 [sway/input/keyboard.c:341] keyboard device 0x61a00006de80 sent event, key=30, down=1

It seems the event first gets processed through the group, whose representative keyboard is 0x61a000048c80, the power button, before coming in as an independent keyboard event to 0x61a00006de80, the actual keyboard.

This means that when the key combo for reload is executed, Sway thinks it came from the power button, since that’s the first event. This ought to be fine, except… printing out which keyboard devices get reset during reload does not print the power button keyboard — so the repeat timer isn’t disarmed. Instead, when Sway reloads, the repeat event on the power button fires, but the binding for it has already been freed. Sway then crashes due to the use-after-free.

I see four issues here with my limited understanding, though some may not actually be issues:

• keymaps_match between Sway and wlroots do not match
• events are sent twice, once through the keyboard group, and once to the actual keyboard
• smart keyboard grouping thinks a power button and a keyboard should be grouped
• power button keyboards do not get reset on reload, probably because they’re not actually added to the seat (?)

(/cc @RedSoxFan based on git blame 😃

All keyboard group are destroyed and recreated on reload

So it turns out this is actually incorrect. The keyboard group associated with the default keymap, default repeat rate, and default repeat delay is not destroyed on reload when there is at least one keyboard that was in the group prior to the reload. All other keyboard groups are destroyed since the inputs gets reset. The use-after-free will only occur if the keyboard that triggered the reload is in that default keyboard group. Submitted #5317.

@Xyene Thanks for digging into this.

To touch on some of your points:

It seems the event first gets processed through the group, whose representative keyboard is 0x61a000048c80, the power button

The keyboard group actually has it’s own keyboard. The log lines that you posted are for the creation of the keyboard group’s sway_keyboard, not the power button’s. There should be a separate line for the power button.

keymaps_match between Sway and wlroots do not match

This should be fine since none of the calls in Sway should be passing in NULL. However, it wouldn’t hurt to add the extra checks.

events are sent twice, once through the keyboard group, and once to the actual keyboard

This is expected and allows for the --input-device=<device> option for bindcode/bindsym. For any keyboard that is in a group, only bindings with a matching --input-device are handled. The rest is handled by the keyboard group event. However, there are some issues that I plan to look into soon (probably this weekend) - #4975 and #4955.

smart keyboard grouping thinks a power button and a keyboard should be grouped

Any input device that is a keyboard that is in the same seat, has the same keymap set, and the same repeat delay and repeat rate set will be grouped together. A power button does emit the event code KEY_POWER (116), which is a keyboard event code. Restricting this to “real keyboards” is non-trivial and is unlikely to be worth the effort.

power button keyboards do not get reset on reload, probably because they’re not actually added to the seat (?)

I’m pretty sure that every input device should be in a seat. If I remember correct, it is actually required that there be a fallback seat and if one doesn’t exist, one will be created. By default seat0 is the implicit fallback seat. You can use swaymsg -t get_seats to see which devices are in each seat.

I believe this is also related to the confusion above about the keyboard group’s keyboard. All keyboard group are destroyed and recreated on reload so there will not be a reset for it. This should be calling sway_keyboard_destroy (which in turn calls sway_keyboard_disarm_key_repeat) during the destroy.

I’ll do some digging and see what I can find.

Can you report the tablet crash on the wlroots issue tracker?

Indeed, bindsym $mod+Shift+c exec "sleep 1; swaymsg reload" works fine for me as well. I guess the problem is that multiple reload commands are being dispatched, and then it tries to reload before it’s done reloading, causing the crash.

Yeah and it’ll point to a different location if you reproduce without debug logging enabled, but it’s still being called while handling keyboard repeat.