xkbcli interactive-x11 crashes with (slightly malformed?) custom keymap

[zackw]

I wrote an XKB keymap from scratch for my laptop, because I wanted total control over what each of the keys did. I'm using sway as the primary window manager/compositor, and the keymap gives the behavior I want with native Wayland clients, but I still need to use a few X11 clients (notably emacs) and the keymap does not quite work as desired when fed to Xwayland. While trying to troubleshoot this, I managed to get xkbcli interactive-x11 to crash on startup:

$ xkbcli interactive-x11
xkbcommon: ERROR: x11: failed to get keymap from X server: unmet condition in get_types(): wire_type->numLevels > 0
xkbcommon: ERROR: x11: failed to get keymap from X server: unmet condition in get_type_names(): type->num_levels == wire_num_levels
Segmentation fault (core dumped)

gdb tells me the crash is actually inside libxkbcommon-x11.so!xkb_x11_keymap_new_from_device. I don't have debugging symbols to hand, unfortunately, so I can't be more specific.

Here is the troublesome custom keymap. I would argue that no matter how out of spec the keymap is libxkbcommon should not crash, but also I would appreciate any comments you may have on what might be wrong with this keymap.

[bluetech]

A crash should definitely not happen. It's probably due to 1b3a1c2.

How do you load the keymap? I tried

$ xkbcomp troublesome-xkb.txt $DISPLAY
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  135 (XKEYBOARD)
  Minor opcode of failed request:  9 (XkbSetMap)
  Value in failed request:  0x8010202
  Serial number of failed request:  27
  Current serial number in output stream:  32

but xkbcli interactive-x11 doesn't crash after that.

[zackw]

How do you load the keymap?

With sway's xkb_file setting. sway loads and parses the keymap, then supplies it to Xwayland using the Wayland keyboard protocol. Xwayland is happy to load the keymap when it receives it this way, but clearly there's still something out-of-spec about it.

[bluetech]

Trying again now I still haven't managed to reproduce this, however it is very likely that the problem is caused by 1b3a1c2 (PR #217), specifically the fact that we no longer short-circuit on error:

libxkbcommon/src/x11/keymap.c

Lines 1175 to 1180 in f8c430c

	bool had_error = false;
	had_error |= !get_map(keymap, conn, get_map_cookie);
	had_error |= !get_indicator_map(keymap, conn, indicator_map_cookie);
	had_error |= !get_compat_map(keymap, conn, compat_map_cookie);
	had_error |= !get_names(keymap, &interner, get_names_cookie);
	had_error |= !get_controls(keymap, conn, get_controls_cookie);

According to the log messages, what is happening is that the keymap fails in get_types (called from get_map), then in get_type_names (called from get_names), either due to the first error or independently. And I assume later some code assumes that a previous call succeeded without checking which triggers the segfault.

We could fix this by being more defensive and keeping the non-short-circuit behavior, but this seems too subtle and hard to maintain to me. So I prefer to go back to short-circuit behavior. @psychon already mentioned what is needed in the PR message:

The new error handling here is obviously untested. To simplify the error handling, I no longer "bail out" after the first error (which would require doing the right amount of xcb_discard_reply() calls), but instead continue through and only "bail out" at the end. IMHO this should not be much of a downside.

so I'll look into doing the needed discard's.

[psychon]

Heh, seems like my PR is the gift that keeps on giving. Sorry for that.

With sway's xkb_file setting.

Now you made me install sway! And Wayland!.....

Anyway, thanks for keeping enough hints on how to reproduce this:

Starting program: /tmp/libxkbcommon/build/xkbcli-interactive-x11 
xkbcommon: ERROR: x11: failed to get keymap from X server: unmet condition in get_types(): wire_type->numLevels > 0
xkbcommon: ERROR: x11: failed to get keymap from X server: unmet condition in get_type_names(): type->num_levels == wire_num_levels

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7fc39ca in get_controls (keymap=0x555555565400, conn=0x55555555d860, cookie=...) at ../src/x11/keymap.c:1127
1127	        keymap->keys[i].repeats = (reply->perKeyRepeat[i / 8] & (1 << (i % 8)));
(gdb) bt
#0  0x00007ffff7fc39ca in get_controls (keymap=0x555555565400, conn=0x55555555d860, cookie=...) at ../src/x11/keymap.c:1127
#1  0x00007ffff7fc3cee in xkb_x11_keymap_new_from_device (ctx=0x55555555c740, conn=0x55555555d860, device_id=3, flags=XKB_KEYMAP_COMPILE_NO_FLAGS) at ../src/x11/keymap.c:1180
#2  0x0000555555556525 in update_keymap (kbd=0x7fffffffe750) at ../tools/interactive-x11.c:128
#3  0x0000555555556637 in init_kbd (kbd=0x7fffffffe750, conn=0x55555555d860, first_xkb_event=85 'U', device_id=3, ctx=0x55555555c740) at ../tools/interactive-x11.c:167
#4  0x0000555555556c7b in main (argc=1, argv=0x7fffffffe898) at ../tools/interactive-x11.c:382
(gdb) bt full
#0  0x00007ffff7fc39ca in get_controls (keymap=0x555555565400, conn=0x55555555d860, cookie=...) at ../src/x11/keymap.c:1127
        i = 0
        reply = 0x555555565360
        __func__ = "get_controls"
#1  0x00007ffff7fc3cee in xkb_x11_keymap_new_from_device (ctx=0x55555555c740, conn=0x55555555d860, device_id=3, flags=XKB_KEYMAP_COMPILE_NO_FLAGS) at ../src/x11/keymap.c:1180
        keymap = 0x555555565400
        format = XKB_KEYMAP_FORMAT_TEXT_V1
        __func__ = "xkb_x11_keymap_new_from_device"
[snip]
(gdb) 

Hm. i=0 says this is the first loop iteration, but that does not seem correct. Also:

(gdb) print reply->perKeyRepeat[0]
$4 = 0 '\000'
(gdb) print keymap->min_key_code
$5 = 0
(gdb) print keymap->max_key_code
$6 = 0

Oh. This means that...

(gdb) print keymap->keys
$9 = (struct xkb_key *) 0x0

Yup. That's it.

Now it's time to test #258...

[bluetech]

Thanks a lot @psychon for reproducing and verifying the fix!

I'll wait a few days in case @zackw will get a chance to test this, then I'll merge & release.