Avoid exposing mime types that couldn't be read from through clipboard

Attempts to address #27

src/xstate/mod.rs

@@ -406,11 +406,13 @@ impl XState {
     }
 
     fn get_atom_name(&self, atom: x::Atom) -> String {
-        self.connection
+        match self
+            .connection
             .wait_for_reply(self.connection.send_request(&x::GetAtomName { atom }))
-            .unwrap()
+        {
-            .name()
+            Ok(reply) => reply.name().to_string(),
-            .to_string()
+            Err(err) => format!("<error getting atom name: {err:?}> {atom:?}"),
+        }
     }
 
     fn get_window_attributes(&self, window: x::Window) -> XResult<WindowAttributes> {

src/xstate/selection.rs

@@ -1,10 +1,11 @@
 use super::XState;
 use crate::server::ForeignSelection;
 use crate::{MimeTypeData, RealServerState};
-use log::{debug, warn};
+use log::{debug, trace, warn};
 use std::rc::Rc;
 use xcb::x;
 
+#[derive(Debug)]
 enum TargetValue {
     U8(Vec<u8>),
     U16(Vec<u16>),
@@ -12,27 +13,30 @@ enum TargetValue {
     Foreign,
 }
 
-pub struct SelectionTarget {
+#[derive(Debug)]
+struct SelectionTargetId {
     name: String,
     atom: x::Atom,
-    value: Option<TargetValue>,
+}
+
+pub struct SelectionTarget {
+    id: SelectionTargetId,
+    value: TargetValue,
 }
 
 impl MimeTypeData for SelectionTarget {
     fn name(&self) -> &str {
-        &self.name
+        &self.id.name
     }
 
     fn data(&self) -> &[u8] {
-        match self.value.as_ref() {
+        match &self.value {
-            Some(TargetValue::U8(v)) => v,
+            TargetValue::U8(v) => v,
             other => {
-                if let Some(other) = other {
+                warn!(
-                    warn!(
                     "Unexpectedly requesting data from mime type with data type {} - nothing will be copied",
                     std::any::type_name_of_val(other)
                 );
-                }
                 &[]
             }
         }
@@ -42,9 +46,9 @@ impl MimeTypeData for SelectionTarget {
 #[derive(Default)]
 pub(crate) struct SelectionData {
     clear_time: Option<u32>,
+    // Selection ID and destination atom
+    tmp_mimes: Vec<(SelectionTargetId, x::Atom)>,
     mime_types: Rc<Vec<SelectionTarget>>,
-    /// List of property on self.wm_window and corresponding index in mime_types
-    mime_destinations: Vec<(x::Atom, usize)>,
     foreign_data: Option<ForeignSelection>,
 }
 
@@ -87,15 +91,18 @@ impl XState {
                     .unwrap();
 
                 SelectionTarget {
-                    name: mime.clone(),
+                    id: SelectionTargetId {
-                    atom: atom.atom(),
+                        name: mime.clone(),
-                    value: Some(TargetValue::Foreign),
+                        atom: atom.atom(),
+                    },
+                    value: TargetValue::Foreign,
                 }
             })
             .collect();
 
         self.selection_data.mime_types = Rc::new(types);
         self.selection_data.foreign_data = Some(selection);
+        trace!("Clipboard set from Wayland");
     }
 
     pub(crate) fn handle_selection_event(
@@ -180,7 +187,7 @@ impl XState {
                             .selection_data
                             .mime_types
                             .iter()
-                            .map(|t| t.atom)
+                            .map(|t| t.id.atom)
                             .collect();
 
                         self.connection
@@ -200,28 +207,35 @@ impl XState {
                             .selection_data
                             .mime_types
                             .iter()
-                            .find(|t| t.atom == other)
+                            .find(|t| t.id.atom == other)
                         else {
-                            debug!("refusing selection requst because given atom could not be found ({other:?})");
+                            if log::log_enabled!(log::Level::Debug) {
+                                let name = self.get_atom_name(other);
+                                debug!("refusing selection request because given atom could not be found ({})", name);
+                            }
                             refuse();
                             return true;
                         };
 
                         macro_rules! set_property {
                             ($data:expr) => {
-                                self.connection
+                                match self.connection.send_and_check_request(&x::ChangeProperty {
-                                    .send_and_check_request(&x::ChangeProperty {
+                                    mode: x::PropMode::Replace,
-                                        mode: x::PropMode::Replace,
+                                    window: e.requestor(),
-                                        window: e.requestor(),
+                                    property: e.property(),
-                                        property: e.property(),
+                                    r#type: target.id.atom,
-                                        r#type: target.atom,
+                                    data: $data,
-                                        data: $data,
+                                }) {
-                                    })
+                                    Ok(_) => success(),
-                                    .unwrap()
+                                    Err(e) => {
+                                        warn!("Failed setting selection property: {e:?}");
+                                        refuse();
+                                    }
+                                }
                             };
                         }
 
-                        match target.value.as_ref().unwrap() {
+                        match &target.value {
                             TargetValue::U8(v) => set_property!(v),
                             TargetValue::U16(v) => set_property!(v),
                             TargetValue::U32(v) => set_property!(v),
@@ -231,12 +245,10 @@ impl XState {
                                     .foreign_data
                                     .as_ref()
                                     .unwrap()
-                                    .receive(target.name.clone(), server_state);
+                                    .receive(target.id.name.clone(), server_state);
                                 set_property!(&data);
                             }
                         }
-
-                        success();
                     }
                 }
             }
@@ -280,6 +292,7 @@ impl XState {
             })
             .collect();
 
+        // Setup target list
         self.connection
             .send_and_check_request(&x::ChangeProperty {
                 mode: x::PropMode::Replace,
@@ -290,6 +303,7 @@ impl XState {
             })
             .unwrap();
 
+        // Request data for our targets
         self.connection
             .send_and_check_request(&x::ConvertSelection {
                 requestor: self.wm_window,
@@ -300,10 +314,9 @@ impl XState {
             })
             .unwrap();
 
-        let (types, dests) = target_props
+        let tmp = target_props
             .chunks_exact(2)
-            .enumerate()
+            .map(|atoms| {
-            .map(|(idx, atoms)| {
                 let [target, property] = atoms.try_into().unwrap();
                 let name = self
                     .connection
@@ -313,26 +326,19 @@ impl XState {
                     )
                     .unwrap();
                 let name = name.name().to_string();
-                let target = SelectionTarget {
+                let target = SelectionTargetId { atom: target, name };
-                    atom: target,
+                (target, property)
-                    name,
-                    value: None,
-                };
-                let dest = (property, idx);
-                (target, dest)
             })
-            .unzip();
+            .collect();
 
-        self.selection_data.mime_types = Rc::new(types);
+        self.selection_data.tmp_mimes = tmp;
-        self.selection_data.mime_destinations = dests;
     }
 
     fn handle_new_clipboard_data(&mut self, server_state: &mut RealServerState) {
-        for (property, idx) in std::mem::take(&mut self.selection_data.mime_destinations) {
+        let mut mime_types = Vec::new();
-            let types = Rc::get_mut(&mut self.selection_data.mime_types).unwrap();
+        for (id, dest) in std::mem::take(&mut self.selection_data.tmp_mimes) {
-            let target = &mut types[idx];
+            let value = {
-            let data = {
+                if id.atom == self.atoms.timestamp {
-                if target.atom == self.atoms.timestamp {
                     TargetValue::U32(vec![self
                         .selection_data
                         .clear_time
@@ -345,7 +351,7 @@ impl XState {
                         .wait_for_reply(self.connection.send_request(&x::GetProperty {
                             delete: true,
                             window: self.wm_window,
-                            property,
+                            property: dest,
                             r#type: x::ATOM_ANY,
                             long_offset: 0,
                             long_length: u32::MAX,
@@ -357,23 +363,27 @@ impl XState {
                         16 => TargetValue::U16(reply.value().to_vec()),
                         32 => TargetValue::U32(reply.value().to_vec()),
                         other => {
-                            let atom = target.atom;
+                            if log::log_enabled!(log::Level::Debug) {
-                            let target = self.get_atom_name(atom);
+                                let atom = id.atom;
-                            let ty = if reply.r#type() == x::ATOM_NONE {
+                                let target = self.get_atom_name(atom);
-                                "None".to_string()
+                                let ty = if reply.r#type() == x::ATOM_NONE {
-                            } else {
+                                    "None".to_string()
-                                self.get_atom_name(reply.r#type())
+                                } else {
-                            };
+                                    self.get_atom_name(reply.r#type())
-                            warn!("unexpected format: {other} (atom: {target}, type: {ty:?}, property: {property:?}) - copies as this type will fail!");
+                                };
+                                debug!("unexpected format: {other} (atom: {target}, type: {ty:?}, property: {dest:?})");
+                            }
                             continue;
                         }
                     }
                 }
             };
 
-            target.value = Some(data);
+            trace!("Selection data: {id:?} {value:?}");
+            mime_types.push(SelectionTarget { id, value });
         }
 
+        self.selection_data.mime_types = Rc::new(mime_types);
         self.connection
             .send_and_check_request(&x::DeleteProperty {
                 window: self.wm_window,
@@ -383,5 +393,6 @@ impl XState {
 
         self.set_clipboard_owner(self.selection_data.clear_time.unwrap());
         server_state.set_copy_paste_source(Rc::clone(&self.selection_data.mime_types));
+        trace!("Clipboard set from X11");
     }
 }

tests/integration.rs

@@ -721,8 +721,8 @@ fn copy_from_x11() {
     assert_ne!(window, owner.owner());
 
     let mimes = f.testwl.data_source_mimes();
-    assert!(mimes.contains(&"text/plain".into())); // mime1
+    assert!(mimes.contains(&"text/plain".into()), "text/plain not in mimes: {mimes:?}"); // mime1
-    assert!(mimes.contains(&"blah/blah".into())); // mime2
+    assert!(mimes.contains(&"blah/blah".into()), "blah/blah not in mimes: {mimes:?}"); // mime2
 
     let data = f.testwl.paste_data();
     f.testwl.dispatch();
@@ -909,3 +909,119 @@ fn different_output_position() {
     assert_eq!(reply.win_x(), 150);
     assert_eq!(reply.win_y(), 12);
 }
+
+#[test]
+fn bad_clipboard_data() {
+    let mut f = Fixture::new();
+    let mut connection = Connection::new(&f.display);
+    let window = connection.new_window(connection.root, 0, 0, 20, 20, false);
+    connection.map_window(window);
+    f.wait_and_dispatch();
+    let surface = f
+        .testwl
+        .last_created_surface_id()
+        .expect("No surface created");
+    f.configure_and_verify_new_toplevel(&mut connection, window, surface);
+
+    connection
+        .send_and_check_request(&x::SetSelectionOwner {
+            owner: window,
+            selection: connection.atoms.clipboard,
+            time: x::CURRENT_TIME,
+        })
+        .unwrap();
+
+    // wait for request to come through
+    std::thread::sleep(std::time::Duration::from_millis(100));
+    let request = match connection.poll_for_event().unwrap() {
+        Some(xcb::Event::X(x::Event::SelectionRequest(r))) => r,
+        other => panic!("Didn't get selection request event, instead got {other:?}"),
+    };
+    assert_eq!(request.target(), connection.atoms.targets);
+    connection.set_property(
+        request.requestor(),
+        x::ATOM_ATOM,
+        request.property(),
+        &[connection.atoms.mime2],
+    );
+    connection
+        .send_and_check_request(&x::SendEvent {
+            propagate: false,
+            destination: x::SendEventDest::Window(request.requestor()),
+            event_mask: x::EventMask::empty(),
+            event: &x::SelectionNotifyEvent::new(
+                request.time(),
+                request.requestor(),
+                request.selection(),
+                request.target(),
+                request.property(),
+            ),
+        })
+        .unwrap();
+
+    std::thread::sleep(std::time::Duration::from_millis(100));
+    let request = match connection.poll_for_event().unwrap() {
+        Some(xcb::Event::X(x::Event::SelectionRequest(r))) => r,
+        other => panic!("Didn't get selection request event, instead got {other:?}"),
+    };
+    assert_eq!(request.target(), connection.atoms.multiple);
+    let pairs = connection
+        .wait_for_reply(connection.send_request(&x::GetProperty {
+            delete: true,
+            window: request.requestor(),
+            property: request.property(),
+            r#type: x::ATOM_ATOM,
+            long_offset: 0,
+            long_length: 4,
+        }))
+        .unwrap();
+
+    let pairs: &[x::Atom] = pairs.value();
+    assert_eq!(pairs.len(), 2);
+    assert!(pairs.contains(&connection.atoms.mime2));
+
+    connection
+        .send_and_check_request(&x::SendEvent {
+            propagate: false,
+            destination: x::SendEventDest::Window(request.requestor()),
+            event_mask: x::EventMask::empty(),
+            event: &x::SelectionNotifyEvent::new(
+                request.time(),
+                request.requestor(),
+                request.selection(),
+                request.target(),
+                request.property(),
+            ),
+        })
+        .unwrap();
+
+    f.wait_and_dispatch();
+    let owner = connection
+        .wait_for_reply(connection.send_request(&x::GetSelectionOwner {
+            selection: connection.atoms.clipboard,
+        }))
+        .unwrap();
+    assert_ne!(window, owner.owner());
+
+    connection
+        .send_and_check_request(&x::ConvertSelection {
+            requestor: window,
+            selection: connection.atoms.clipboard,
+            target: connection.atoms.mime2,
+            property: connection.atoms.mime1,
+            time: x::CURRENT_TIME,
+        })
+        .unwrap();
+
+    std::thread::sleep(std::time::Duration::from_millis(100));
+
+    let mut e = None;
+    while let Some(event) = connection.poll_for_event().unwrap() {
+        if let xcb::Event::X(x::Event::SelectionNotify(event)) = event {
+            e = Some(event);
+            break;
+        }
+    }
+    let e = e.expect("No selection notify event");
+    assert_eq!(e.property(), x::ATOM_NONE);
+}