Wlroots: Segfault during copy&paste operations into Xwayland

[luispabon]

• Sway Version:
  sway version 1.5-32b93ef6 (Nov 28 2020, branch 'master')
  wlroots 154fe869 (Nov 19)

• Debug Log:
  The original log is 27M as it was a debug session that had been going on for a couple of days. The following has the first 3k and last 3k lines of that log:
  https://paste.ubuntu.com/p/NwkzHgjVn9/

• Configuration File:
  https://github.com/luispabon/sway-dotfiles/tree/master/configs/sway

• Stack Trace:
  https://paste.ubuntu.com/p/jWfQpXDHcV/

• Description:
  This segfault feels similar to another issue that was patched before and could potentially be a regression:
  https://github.com/swaywm/wlroots/issues/2425

Apologies if I put the issue in the sway bug tracker and not wlroots as I have no idea if it belongs there.

Same steps: session going for a day or two, pasting into xwayland stops functioning - xwayland apps hang for a few seconds when you paste into them, kill xwayland, start another xwayland app and try to paste again, then hang again for a couple of seconds and segfault.

[Xyene]

This looks like a use-after-free. The bug sounds like when xwayland dies, we don't cancel queued transfers. When xwayland comes back up and you paste again, after the paste completes we look at the queued transfers, see the old one (from the previous xwayland run), and try to reschedule it.

At that point, the xwm the transfer references (transfer->selection->xwm) points to invalid memory, so https://github.com/swaywm/wlroots/blob/de896caceb9f6354a99c9435c3a429ff52b6ca4a/xwayland/selection/outgoing.c#L233-L234 returns NULL.

Could you:

1. add wlr_log(WLR_INFO, "xwm=%p, xwm->xwayland=%p, xwm->xwayland->wl_display", xwm, xwm->xwayland, xwm->xwayland->wl_display); right after the log on line 235 of that file; and
2. compile with ASan enabled?

I remember encountering this issue once while fixing https://github.com/swaywm/wlroots/pull/2427 -- it's not a regression, but I wasn't able to reproduce it since then.

[Xyene]

Here's an untested patch to wlroots you could try (ideally with the extra log from my previous message, and with ASan):

diff --git a/include/xwayland/selection.h b/include/xwayland/selection.h
index 67a23e81..7f244555 100644
--- a/include/xwayland/selection.h
+++ b/include/xwayland/selection.h
@@ -47,6 +47,8 @@ void xwm_selection_transfer_close_source_fd(
    struct wlr_xwm_selection_transfer *transfer);
 void xwm_selection_transfer_destroy_property_reply(
    struct wlr_xwm_selection_transfer *transfer);
+void xwm_selection_transfer_destroy_outgoing(
+   struct wlr_xwm_selection_transfer *transfer);

 xcb_atom_t xwm_mime_type_to_atom(struct wlr_xwm *xwm, char *mime_type);
 char *xwm_mime_type_from_atom(struct wlr_xwm *xwm, xcb_atom_t atom);
@@ -72,5 +74,6 @@ void xwm_seat_handle_start_drag(struct wlr_xwm *xwm, struct wlr_drag *drag);

 void xwm_selection_init(struct wlr_xwm *xwm);
 void xwm_selection_finish(struct wlr_xwm *xwm);
+void xwm_selection_destroy_all(struct wlr_xwm *xwm);

 #endif
diff --git a/xwayland/selection/outgoing.c b/xwayland/selection/outgoing.c
index a5ce2fd2..b0abf95e 100644
--- a/xwayland/selection/outgoing.c
+++ b/xwayland/selection/outgoing.c
@@ -66,7 +66,7 @@ static struct wlr_xwm_selection_transfer *xwm_selection_transfer_get_first(
    return first;
 }

-static void xwm_selection_transfer_destroy_outgoing(
+void xwm_selection_transfer_destroy_outgoing(
        struct wlr_xwm_selection_transfer *transfer) {
    struct wlr_xwm_selection *selection = transfer->selection;
    bool was_first = transfer == xwm_selection_transfer_get_first(selection);
diff --git a/xwayland/selection/selection.c b/xwayland/selection/selection.c
index a8cb5df4..282a1583 100644
--- a/xwayland/selection/selection.c
+++ b/xwayland/selection/selection.c
@@ -238,6 +238,30 @@ void xwm_selection_finish(struct wlr_xwm *xwm) {
    }
 }

+void xwm_selection_destroy_all(struct wlr_xwm *xwm) {
+   if (!xwm) {
+       return;
+   }
+
+   xwm_selection_finish(xwm);
+
+   struct wlr_xwm_selection *selections[] = {
+       &xwm->clipboard_selection,
+       &xwm->primary_selection,
+       &xwm->dnd_selection,
+   };
+
+   for (size_t i = 0; i < sizeof(selections)/sizeof(selections[0]); ++i) {
+       struct wlr_xwm_selection *selection = selections[i];
+
+       struct wlr_xwm_selection_transfer *outgoing, *tmp;
+       wl_list_for_each_safe(outgoing, tmp, &selection->outgoing, outgoing_link) {
+           wlr_log(WLR_INFO, "destroyed pending transfer %ld/%p", i, outgoing);
+           xwm_selection_transfer_destroy_outgoing(outgoing);
+       }
+   }
+}
+
 static void xwm_selection_set_owner(struct wlr_xwm_selection *selection,
        bool set) {
    if (set) {
diff --git a/xwayland/xwm.c b/xwayland/xwm.c
index 781bf15d..97174b4e 100644
--- a/xwayland/xwm.c
+++ b/xwayland/xwm.c
@@ -1607,7 +1607,7 @@ void xwm_destroy(struct wlr_xwm *xwm) {
    if (!xwm) {
        return;
    }
-   xwm_selection_finish(xwm);
+   xwm_selection_destroy_all(xwm);
    if (xwm->cursor) {
        xcb_free_cursor(xwm->xcb_conn, xwm->cursor);
    }

I think this should prevent queued transfers outliving the Xwayland instance they started in.

[Xyene]

@luispabon, did you have a chance to try the patch above?

[luispabon]

I did, I have it applied and have seen no further segfaults. They did happen very rarely after a prior fix to a similar issue but looks like this might have done it.

Apologies I forgot to report back here.