Switch from xcb crate to x11rb (#173)

Replace the `xcb` and `xcb-util` crates with `x11rb`. We were using an old version of the `xcb` crate which had some soundness issue. `x11rb` doesn't have these issues and generally provides a safer and nicer to use API.

It's possible to use `x11rb` without linking to xcb at all, using the `RustConnection` API, but unfortunately we have to use the `XCBConnection` API (which uses xcb under the hood) due to our use of the xlib GLX API for creating OpenGL contexts. In the future, it might be possible to avoid linking to xlib and xcb by replacing GLX with EGL.

Getting the xlib-xcb integration to work also necessitated upgrading the version of the `x11` crate, since the version we were using was missing some necessary functionality that was previously being provided by the `xcb` crate.

Author: micahrj

Cargo.toml

@@ -23,9 +23,8 @@ keyboard-types = { version = "0.6.1", default-features = false }
 raw-window-handle = "0.5"
 
 [target.'cfg(target_os="linux")'.dependencies]
-xcb = { version = "0.9", features = ["thread", "xlib_xcb", "dri2"] }
-x11 = { version = "2.18", features = ["xlib", "xcursor"] }
-xcb-util = { version = "0.3", features = ["icccm"] }
+x11rb = { version = "0.13.0", features = ["cursor", "resource_manager", "allow-unsafe-code"] }
+x11 = { version = "2.21", features = ["xlib", "xcursor", "xlib_xcb"] }
 nix = "0.22.0"
 
 [target.'cfg(target_os="windows")'.dependencies]

src/x11/cursor.rs

@@ -1,105 +1,100 @@
-use std::os::raw::c_char;
+use std::error::Error;
+
+use x11rb::connection::Connection;
+use x11rb::cursor::Handle as CursorHandle;
+use x11rb::protocol::xproto::{ConnectionExt as _, Cursor};
+use x11rb::xcb_ffi::XCBConnection;
 
 use crate::MouseCursor;
 
-fn create_empty_cursor(display: *mut x11::xlib::Display) -> Option<u32> {
-    let data = 0;
-    let pixmap = unsafe {
-        let screen = x11::xlib::XDefaultScreen(display);
-        let window = x11::xlib::XRootWindow(display, screen);
-        x11::xlib::XCreateBitmapFromData(display, window, &data, 1, 1)
-    };
+fn create_empty_cursor(conn: &XCBConnection, screen: usize) -> Result<Cursor, Box<dyn Error>> {
+    let cursor_id = conn.generate_id()?;
+    let pixmap_id = conn.generate_id()?;
+    let root_window = conn.setup().roots[screen].root;
+    conn.create_pixmap(1, pixmap_id, root_window, 1, 1)?;
+    conn.create_cursor(cursor_id, pixmap_id, pixmap_id, 0, 0, 0, 0, 0, 0, 0, 0)?;
+    conn.free_pixmap(pixmap_id)?;
 
-    if pixmap == 0 {
-        return None;
-    }
-
-    unsafe {
-        // We don't care about this color, since it only fills bytes
-        // in the pixmap which are not 0 in the mask.
-        let mut color: x11::xlib::XColor = std::mem::zeroed();
-
-        let cursor = x11::xlib::XCreatePixmapCursor(
-            display,
-            pixmap,
-            pixmap,
-            &mut color as *mut _,
-            &mut color as *mut _,
-            0,
-            0,
-        );
-        x11::xlib::XFreePixmap(display, pixmap);
-
-        Some(cursor as u32)
-    }
+    Ok(cursor_id)
 }
 
-fn load_cursor(display: *mut x11::xlib::Display, name: &[u8]) -> Option<u32> {
-    let xcursor =
-        unsafe { x11::xcursor::XcursorLibraryLoadCursor(display, name.as_ptr() as *const c_char) };
-
-    if xcursor == 0 {
-        None
+fn load_cursor(
+    conn: &XCBConnection, cursor_handle: &CursorHandle, name: &str,
+) -> Result<Option<Cursor>, Box<dyn Error>> {
+    let cursor = cursor_handle.load_cursor(conn, name)?;
+    if cursor != x11rb::NONE {
+        Ok(Some(cursor))
     } else {
-        Some(xcursor as u32)
+        Ok(None)
     }
 }
 
-fn load_first_existing_cursor(display: *mut x11::xlib::Display, names: &[&[u8]]) -> Option<u32> {
-    names
-        .iter()
-        .map(|name| load_cursor(display, name))
-        .find(|xcursor| xcursor.is_some())
-        .unwrap_or(None)
+fn load_first_existing_cursor(
+    conn: &XCBConnection, cursor_handle: &CursorHandle, names: &[&str],
+) -> Result<Option<Cursor>, Box<dyn Error>> {
+    for name in names {
+        let cursor = load_cursor(conn, cursor_handle, name)?;
+        if cursor.is_some() {
+            return Ok(cursor);
+        }
+    }
+
+    Ok(None)
 }
 
-pub(super) fn get_xcursor(display: *mut x11::xlib::Display, cursor: MouseCursor) -> u32 {
-    let load = |name: &[u8]| load_cursor(display, name);
-    let loadn = |names: &[&[u8]]| load_first_existing_cursor(display, names);
+pub(super) fn get_xcursor(
+    conn: &XCBConnection, screen: usize, cursor_handle: &CursorHandle, cursor: MouseCursor,
+) -> Result<Cursor, Box<dyn Error>> {
+    let load = |name: &str| load_cursor(conn, cursor_handle, name);
+    let loadn = |names: &[&str]| load_first_existing_cursor(conn, cursor_handle, names);
 
     let cursor = match cursor {
         MouseCursor::Default => None, // catch this in the fallback case below
 
-        MouseCursor::Hand => loadn(&[b"hand2\0", b"hand1\0"]),
-        MouseCursor::HandGrabbing => loadn(&[b"closedhand\0", b"grabbing\0"]),
-        MouseCursor::Help => load(b"question_arrow\0"),
-
-        MouseCursor::Hidden => create_empty_cursor(display),
-
-        MouseCursor::Text => loadn(&[b"text\0", b"xterm\0"]),
-        MouseCursor::VerticalText => load(b"vertical-text\0"),
-
-        MouseCursor::Working => load(b"watch\0"),
-        MouseCursor::PtrWorking => load(b"left_ptr_watch\0"),
-
-        MouseCursor::NotAllowed => load(b"crossed_circle\0"),
-        MouseCursor::PtrNotAllowed => loadn(&[b"no-drop\0", b"crossed_circle\0"]),
-
-        MouseCursor::ZoomIn => load(b"zoom-in\0"),
-        MouseCursor::ZoomOut => load(b"zoom-out\0"),
-
-        MouseCursor::Alias => load(b"link\0"),
-        MouseCursor::Copy => load(b"copy\0"),
-        MouseCursor::Move => load(b"move\0"),
-        MouseCursor::AllScroll => load(b"all-scroll\0"),
-        MouseCursor::Cell => load(b"plus\0"),
-        MouseCursor::Crosshair => load(b"crosshair\0"),
-
-        MouseCursor::EResize => load(b"right_side\0"),
-        MouseCursor::NResize => load(b"top_side\0"),
-        MouseCursor::NeResize => load(b"top_right_corner\0"),
-        MouseCursor::NwResize => load(b"top_left_corner\0"),
-        MouseCursor::SResize => load(b"bottom_side\0"),
-        MouseCursor::SeResize => load(b"bottom_right_corner\0"),
-        MouseCursor::SwResize => load(b"bottom_left_corner\0"),
-        MouseCursor::WResize => load(b"left_side\0"),
-        MouseCursor::EwResize => load(b"h_double_arrow\0"),
-        MouseCursor::NsResize => load(b"v_double_arrow\0"),
-        MouseCursor::NwseResize => loadn(&[b"bd_double_arrow\0", b"size_bdiag\0"]),
-        MouseCursor::NeswResize => loadn(&[b"fd_double_arrow\0", b"size_fdiag\0"]),
-        MouseCursor::ColResize => loadn(&[b"split_h\0", b"h_double_arrow\0"]),
-        MouseCursor::RowResize => loadn(&[b"split_v\0", b"v_double_arrow\0"]),
+        MouseCursor::Hand => loadn(&["hand2", "hand1"])?,
+        MouseCursor::HandGrabbing => loadn(&["closedhand", "grabbing"])?,
+        MouseCursor::Help => load("question_arrow")?,
+
+        MouseCursor::Hidden => Some(create_empty_cursor(conn, screen)?),
+
+        MouseCursor::Text => loadn(&["text", "xterm"])?,
+        MouseCursor::VerticalText => load("vertical-text")?,
+
+        MouseCursor::Working => load("watch")?,
+        MouseCursor::PtrWorking => load("left_ptr_watch")?,
+
+        MouseCursor::NotAllowed => load("crossed_circle")?,
+        MouseCursor::PtrNotAllowed => loadn(&["no-drop", "crossed_circle"])?,
+
+        MouseCursor::ZoomIn => load("zoom-in")?,
+        MouseCursor::ZoomOut => load("zoom-out")?,
+
+        MouseCursor::Alias => load("link")?,
+        MouseCursor::Copy => load("copy")?,
+        MouseCursor::Move => load("move")?,
+        MouseCursor::AllScroll => load("all-scroll")?,
+        MouseCursor::Cell => load("plus")?,
+        MouseCursor::Crosshair => load("crosshair")?,
+
+        MouseCursor::EResize => load("right_side")?,
+        MouseCursor::NResize => load("top_side")?,
+        MouseCursor::NeResize => load("top_right_corner")?,
+        MouseCursor::NwResize => load("top_left_corner")?,
+        MouseCursor::SResize => load("bottom_side")?,
+        MouseCursor::SeResize => load("bottom_right_corner")?,
+        MouseCursor::SwResize => load("bottom_left_corner")?,
+        MouseCursor::WResize => load("left_side")?,
+        MouseCursor::EwResize => load("h_double_arrow")?,
+        MouseCursor::NsResize => load("v_double_arrow")?,
+        MouseCursor::NwseResize => loadn(&["bd_double_arrow", "size_bdiag"])?,
+        MouseCursor::NeswResize => loadn(&["fd_double_arrow", "size_fdiag"])?,
+        MouseCursor::ColResize => loadn(&["split_h", "h_double_arrow"])?,
+        MouseCursor::RowResize => loadn(&["split_v", "v_double_arrow"])?,
     };
 
-    cursor.or_else(|| load(b"left_ptr\0")).unwrap_or(0)
+    if let Some(cursor) = cursor {
+        Ok(cursor)
+    } else {
+        Ok(load("left_ptr")?.unwrap_or(x11rb::NONE))
+    }
 }

src/x11/keyboard.rs

@@ -18,7 +18,7 @@
 
 //! X11 keyboard handling
 
-use xcb::xproto;
+use x11rb::protocol::xproto::{KeyButMask, KeyPressEvent, KeyReleaseEvent};
 
 use keyboard_types::*;
 
@@ -361,43 +361,43 @@ fn hardware_keycode_to_code(hw_keycode: u16) -> Code {
 }
 
 // Extracts the keyboard modifiers from, e.g., the `state` field of
-// `xcb::xproto::ButtonPressEvent`
-pub(super) fn key_mods(mods: u16) -> Modifiers {
+// `x11rb::protocol::xproto::ButtonPressEvent`
+pub(super) fn key_mods(mods: KeyButMask) -> Modifiers {
     let mut ret = Modifiers::default();
-    let mut key_masks = [
-        (xproto::MOD_MASK_SHIFT, Modifiers::SHIFT),
-        (xproto::MOD_MASK_CONTROL, Modifiers::CONTROL),
+    let key_masks = [
+        (KeyButMask::SHIFT, Modifiers::SHIFT),
+        (KeyButMask::CONTROL, Modifiers::CONTROL),
         // X11's mod keys are configurable, but this seems
         // like a reasonable default for US keyboards, at least,
         // where the "windows" key seems to be MOD_MASK_4.
-        (xproto::MOD_MASK_1, Modifiers::ALT),
-        (xproto::MOD_MASK_2, Modifiers::NUM_LOCK),
-        (xproto::MOD_MASK_4, Modifiers::META),
-        (xproto::MOD_MASK_LOCK, Modifiers::CAPS_LOCK),
+        (KeyButMask::BUTTON1, Modifiers::ALT),
+        (KeyButMask::BUTTON2, Modifiers::NUM_LOCK),
+        (KeyButMask::BUTTON4, Modifiers::META),
+        (KeyButMask::LOCK, Modifiers::CAPS_LOCK),
     ];
-    for (mask, modifiers) in &mut key_masks {
-        if mods & (*mask as u16) != 0 {
+    for (mask, modifiers) in &key_masks {
+        if mods.contains(*mask) {
             ret |= *modifiers;
         }
     }
     ret
 }
 
-pub(super) fn convert_key_press_event(key_press: &xcb::KeyPressEvent) -> KeyboardEvent {
-    let hw_keycode = key_press.detail();
+pub(super) fn convert_key_press_event(key_press: &KeyPressEvent) -> KeyboardEvent {
+    let hw_keycode = key_press.detail;
     let code = hardware_keycode_to_code(hw_keycode.into());
-    let modifiers = key_mods(key_press.state());
+    let modifiers = key_mods(key_press.state);
     let key = code_to_key(code, modifiers);
     let location = code_to_location(code);
     let state = KeyState::Down;
 
     KeyboardEvent { code, key, modifiers, location, state, repeat: false, is_composing: false }
 }
 
-pub(super) fn convert_key_release_event(key_release: &xcb::KeyReleaseEvent) -> KeyboardEvent {
-    let hw_keycode = key_release.detail();
+pub(super) fn convert_key_release_event(key_release: &KeyReleaseEvent) -> KeyboardEvent {
+    let hw_keycode = key_release.detail;
     let code = hardware_keycode_to_code(hw_keycode.into());
-    let modifiers = key_mods(key_release.state());
+    let modifiers = key_mods(key_release.state);
     let key = code_to_key(code, modifiers);
     let location = code_to_location(code);
     let state = KeyState::Up;