wayvnc crashes sway/wayfire

[jbeich]

Environment:

$ uname -a
FreeBSD 13.0-CURRENT #1 r357254: Wed Jan 29 14:31:23 UTC 2020 foo@bar:/usr/src/amd64.amd64/sys/MYKERNEL amd64

$ pkg info -x libffi xkbcommon \^wayland drm-.\*kmod
libffi-3.2.1_3
libxkbcommon-0.8.4
wayland-1.17.0
wayland-protocols-1.18
drm-devel-kmod-5.0.g20200115

$ sway --version
sway version 1.2-rc1-199-g1dbc7f75
wlroots version 0.10.0-6-gf2943bdf

Steps to reproduce:

$ echo "exec wayvnc" >/tmp/sway.conf
$ env -i WAYLAND_DISPLAY=$WAYLAND_DISPLAY XDG_RUNTIME_DIR=$XDG_RUNTIME_DIR HOME=/tmp \
  sway -dc /tmp/sway.conf
[...]
[sway/commands.c:255] Handling command 'exec wayvnc'
[sway/commands/exec_always.c:46] Executing wayvnc
[sway/commands/exec_always.c:87] Child process created with pid 32964
[sway/tree/root.c:290] Recording workspace for process 32964
[sway/server.c:208] Running compositor on wayland display 'wayland-1'
[GLES2] FS SIMD16 shader: 2 inst, 0 loops, 0 cycles, 0:0 spills:fills, Promoted 0 constants, compacted 32 to 32 bytes.
[sway/desktop/layer_shell.c:178] Usable area changed, rearranging output
[sway/tree/arrange.c:264] Usable area for ws: 1770x710@0,0
[sway/tree/arrange.c:294] Arranging workspace '1' at 0.000000, 0.000000
[sway/tree/arrange.c:264] Usable area for ws: 1770x710@0,0
[sway/tree/arrange.c:294] Arranging workspace '1' at 0.000000, 0.000000
[sway/desktop/transaction.c:412] Transaction 0x8037a98c0 committing with 1 instructions
[sway/desktop/transaction.c:280] Applying transaction 0x8037a98c0
[sway/tree/arrange.c:264] Usable area for ws: 1770x710@0,0
[sway/tree/arrange.c:294] Arranging workspace '1' at 0.000000, 0.000000
[sway/desktop/transaction.c:412] Transaction 0x8037a98c0 committing with 1 instructions
[sway/desktop/transaction.c:280] Applying transaction 0x8037a98c0
[GLES2] FS SIMD16 shader: 2 inst, 0 loops, 0 cycles, 0:0 spills:fills, Promoted 0 constants, compacted 32 to 32 bytes.
[sway/input/input-manager.c:320] adding virtual keyboard: '0:0:virtual_keyboard'
[sway/input/seat.c:817] adding device 0:0:virtual_keyboard to seat seat0
[sway/input/keyboard.c:755] Adding keyboard 0:0:virtual_keyboard to group 0x801ef2700
ERROR: The compositor is gone. Exiting...
Bus error

* thread #1, name = 'sway', stop reason = signal SIGBUS: hardware error
    frame #0: 0x0000000800b92fcf libc.so.7`strlen(str="") at strlen.c:101:8
   98            * boundaries is integral multiple of word size.
   99            */
   100          lp = (const unsigned long *)((uintptr_t)str & ~LONGPTR_MASK);
-> 101          va = (*lp - mask01);
   102          vb = ((~*lp) & mask80);
   103          lp++;
   104          if (va & vb)
(lldb) bt
* thread #1, name = 'sway', stop reason = signal SIGBUS: hardware error
  * frame #0: 0x0000000800b92fcf libc.so.7`strlen(str="") at strlen.c:101:8
    frame #1: 0x000000080096c0d2 libxkbcommon.so.0`xkb_keymap_new_from_string(ctx=0x0000000803571e00, string="", format=XKB_KEYMAP_FORMAT_TEXT_V1, flags=XKB_KEYMAP_COMPILE_NO_FLAGS) at keymap.c:162:52
    frame #2: 0x00000008008fe939 libwlroots.so.5`virtual_keyboard_keymap(client=0x00000008034d6f00, resource=0x0000000801eca500, format=1, fd=23, size=44984) at wlr_virtual_keyboard_v1.c:57:30
    frame #3: 0x000000080141ca54 libffi.so.6`ffi_call_unix64 at unix64.S:76
    frame #4: 0x000000080141bdea libffi.so.6`ffi_call(cif=0x00007fffffffe760, fn=(libwlroots.so.5`virtual_keyboard_keymap at wlr_virtual_keyboard_v1.c:45), rvalue=0x0000000000000000, avalue=0x00007fffffffe790) at ffi64.c:525:3
    frame #5: 0x0000000800846542 libwayland-server.so.0`wl_closure_invoke(closure=0x0000000801ed2840, flags=2, target=0x0000000801eca500, opcode=0, data=0x00000008034d6f00) at connection.c:1014:2
    frame #6: 0x000000080083efc7 libwayland-server.so.0`wl_client_connection_data(fd=21, mask=1, data=0x00000008034d6f00) at wayland-server.c:448:4
    frame #7: 0x0000000800842f87 libwayland-server.so.0`wl_event_source_fd_dispatch(source=0x000000080355a2c0, ep=0x00007fffffffea10) at event-loop.c:95:9
    frame #8: 0x0000000800843b65 libwayland-server.so.0`wl_event_loop_dispatch(loop=0x0000000801eb6b90, timeout=-1) at event-loop.c:641:4
    frame #9: 0x00000008008403cf libwayland-server.so.0`wl_display_run(display=0x0000000801ed0000) at wayland-server.c:1333:3
    frame #10: 0x0000000000237c84 sway`server_run(server=0x0000000000296008) at server.c:209:2
    frame #11: 0x0000000000236ce5 sway`main(argc=3, argv=0x00007fffffffecf8) at main.c:404:2
    frame #12: 0x00000000002260ff sway`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1.c:76:7
(lldb) f 2
frame #2: 0x00000008008fe939 libwlroots.so.5`virtual_keyboard_keymap(client=0x00000008034d6f00, resource=0x0000000801eca500, format=1, fd=23, size=44984) at wlr_virtual_keyboard_v1.c:57:30
   54           if (data == MAP_FAILED) {
   55                   goto fd_fail;
   56           }
-> 57           struct xkb_keymap *keymap = xkb_keymap_new_from_string(context, data,
   58                   XKB_KEYMAP_FORMAT_TEXT_V1, XKB_KEYMAP_COMPILE_NO_FLAGS);
   59           munmap(data, size);
   60           if (!keymap) {

---

wlroots has migrated to gitlab.freedesktop.org. This issue has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/2014

[valpackett]

Same still happens with libxkbcommon-0.10.0, wayland-1.18.0, wlroots & wayfire both master as of right now

[valpackett]

So wayvnc was doing ftruncate(0) instead of ftruncate(size) on the file descriptor, and it worked on Linux because shm_open returns a regular file there: https://github.com/any1/wayvnc/pull/31

In our case, we were getting a fresh shm fd with nothing written to it, so basically uninitialized memory.

Maaaybe wlroots should validate that the incoming file is actually a string, i.e. that there's at least one zero byte in the size bytes from the mmapped address.

Ideally libxkbcommon would take a length parameter to xkb_keymap_new_from_string :)

[emersion]

Ah, indeed. We can use xkb_keymap_new_from_buffer instead. And checking for a final \0 is a good idea.

[valpackett]

hm, the size wayvnc uses does not include the final \0, it just allocates strlen(keymap_string) bytes. Which seems like a bug (for comparison, types/wlr_keyboard.c does strlen() + 1.. // upd: https://github.com/any1/wayvnc/pull/32) but that means if we add a hard check now, existing versions of wayvnc would be broken.

// virtual-keyboard-unstable-v1.xml actually doesn't specify anything about that, neither does wayland.xml, that's kinda sad

[emersion]

The Wayland protocol says it's the keymap size in bytes. The keymap isn't defined as a string, it's just an opaque buffer from the protocol point-of-view. So it does include the final NULL byte.

[progandy]

The Wayland protocol says it's the keymap size in bytes. The keymap isn't defined as a string, it's just an opaque buffer from the protocol point-of-view. So it does include the final NULL byte.

Isn't it possible to have the keymap stored in a textfile and just open that and send its filedescriptor to wlroots? In that case there wouldn't be a final NULL, right?

[emersion]

Isn't it possible to have the keymap stored in a textfile and just open that and send its filedescriptor to wlroots? In that case there wouldn't be a final NULL, right?

That's not possible, because the file could contain references to other files. Keymaps sent via Wayland need to be self-contained.

[progandy]

You can have a custom keymap in a self-contained file. In my opinion the delimiting NULL is an implementation detail that doesn't belong to the keymap itself, but to C strings. Since the protocol doesn't make any mention of that and it does provide the size in bytes, just use that and not bother with the NULL.

And in the unlikely future, libxkbcommon might even support keymaps in the compiled xkm format. (In my case that would reduce the size wayland has to shuffle around by more than half from 64 KB (with whitespace) or 40KB (without whitespace) to 13KB, but I guess that ship has sailed since clients already expect the textual representation)

[emersion]

You can have a custom keymap in a self-contained file.

There are more issues, e.g. clients could write to the file. Making FDs read-only require platform-specific mechanisms and a recent enough wl_seat version for all clients.

In my opinion the delimiting NULL is an implementation detail that doesn't belong to the keymap itself, but to C strings. Since the protocol doesn't make any mention of that and it does provide the size in bytes, just use that and not bother with the NULL.

The protocol doesn't care about the final NULL byte, yes. However the libxkbcommon format does care. As soon as WL_KEYBOARD_KEYMAP_FORMAT_XKB_V1 is used a final NULL byte is expected.

And in the unlikely future, libxkbcommon might even support keymaps in the compiled xkm format. (In my case that would reduce the size wayland has to shuffle around by more than half from 64 KB (with whitespace) or 40KB (without whitespace) to 13KB, but I guess that ship has sailed since clients already expect the textual representation)

Since shared memory is used, it's not that of a big deal.