From cddc1c1bd9f796709c50f4bbb300788edd42fd4f Mon Sep 17 00:00:00 2001 From: Alexander Orzechowski Date: Thu, 20 Jan 2022 09:39:13 -0500 Subject: [PATCH] xdg-foreign: Fix crash on destroy of degenerate surface I am running a custom compiled version of chromium with a patch to get it up and running on sway git at the moment, and in that development build I compiled there is a bug where the browser will crash if you try to open a file select dialog. When this crash happens, chromium will not close, but instead will remain open and impossible to close unless you send a SIGKILL signal to the process. However, sway will crash to tty when you send the SIGKILL. I have a hunch that when chromium is opening the file select dialog it is creating some sort of a xdg toplevel surface. But it freezes before it fully initializes the surface. When the SIGKILL signal is given, sway/wlroots will try to free the xdg_toplevel surface but because it hasn't fully initialized due to the frozen window, it segfaults. Don't be fooled by the assert, the assert is not firing, the surface pointer is indeed NULL here. * thread #1, name = 'sway', stop reason = signal SIGSEGV: invalid address (fault address: 0x28) frame #0: 0x00007ffff78b9041 libwlroots.so.11`wlr_xdg_toplevel_set_parent(surface=0x0000000000000000, parent=0x0000000000000000) at wlr_xdg_toplevel.c:159:37 156 157 void wlr_xdg_toplevel_set_parent(struct wlr_xdg_surface *surface, 158 struct wlr_xdg_surface *parent) { -> 159 assert(surface->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL); 160 assert(!parent || parent->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL); 161 162 if (surface->toplevel->parent) { (lldb) up error: sway {0x0003442a}: DIE has DW_AT_ranges(DW_FORM_sec_offset 0x67) attribute, but range extraction failed (invalid range list offset 0x67), please file a bug and attach the file at the start of this error message error: sway {0x0003442a}: DIE has DW_AT_ranges(DW_FORM_sec_offset 0x67) attribute, but range extraction failed (invalid range list offset 0x67), please file a bug and attach the file at the start of this error message frame #1: 0x00007ffff78e176e libwlroots.so.11`destroy_imported(imported=0x000055555626d570) at wlr_xdg_foreign_v1.c:154:3 151 wl_list_for_each_safe(child, child_tmp, &imported->children, link) { 152 struct wlr_xdg_surface *xdg_child = 153 wlr_xdg_surface_from_wlr_surface(child->surface); -> 154 wlr_xdg_toplevel_set_parent(xdg_child, NULL); 155 } 156 157 wl_list_remove(&imported->exported_destroyed.link); (lldb) up frame #2: 0x00007ffff78e1b9d libwlroots.so.11`xdg_imported_handle_resource_destroy(resource=0x00005555562555a0) at wlr_xdg_foreign_v1.c:280:2 277 struct wl_resource *resource) { 278 struct wlr_xdg_imported_v1 *imported = xdg_imported_from_resource(resource); 279 if (!imported) { -> 280 return; 281 } 282 283 destroy_imported(imported); (lldb) up frame #3: 0x00007ffff794989a libwayland-server.so.0`___lldb_unnamed_symbol211 + 154 libwayland-server.so.0`___lldb_unnamed_symbol211: -> 0x7ffff794989a <+154>: andl $0x1, %r13d 0x7ffff794989e <+158>: je 0x7ffff79498b0 ; <+176> 0x7ffff79498a0 <+160>: addq $0x8, %rsp 0x7ffff79498a4 <+164>: movl $0x1, %eax (lldb) up frame #4: 0x00007ffff794fec0 libwayland-server.so.0`___lldb_unnamed_symbol290 + 64 libwayland-server.so.0`___lldb_unnamed_symbol290: -> 0x7ffff794fec0 <+64>: cmpl $0x1, %eax 0x7ffff794fec3 <+67>: jne 0x7ffff794fed3 ; <+83> 0x7ffff794fec5 <+69>: addq $0x8, %rbx 0x7ffff794fec9 <+73>: cmpq %rbx, %r13 (lldb) up frame #5: 0x00007ffff79503e0 libwayland-server.so.0`___lldb_unnamed_symbol300 + 32 libwayland-server.so.0`___lldb_unnamed_symbol300: -> 0x7ffff79503e0 <+32>: cmpl $0x1, %eax 0x7ffff79503e3 <+35>: je 0x7ffff79503f0 ; <+48> 0x7ffff79503e5 <+37>: popq %rbx 0x7ffff79503e6 <+38>: popq %r12 (lldb) up frame #6: 0x00007ffff794a30e libwayland-server.so.0`wl_client_destroy + 126 libwayland-server.so.0`wl_client_destroy: -> 0x7ffff794a30e <+126>: movq %r12, %rdi 0x7ffff794a311 <+129>: callq 0x7ffff7950150 ; ___lldb_unnamed_symbol293 0x7ffff794a317 <+135>: movq 0x8(%rbp), %rdi 0x7ffff794a31b <+139>: callq *0xdc77(%rip) (lldb) up frame #7: 0x00007ffff794a3f7 libwayland-server.so.0`___lldb_unnamed_symbol214 + 119 libwayland-server.so.0`___lldb_unnamed_symbol214: -> 0x7ffff794a3f7 <+119>: movq 0x28(%rsp), %rax 0x7ffff794a3fc <+124>: subq %fs:0x28, %rax 0x7ffff794a405 <+133>: jne 0x7ffff794a727 ; <+935> 0x7ffff794a40b <+139>: addq $0x38, %rsp (lldb) up frame #8: 0x00007ffff794d1ca libwayland-server.so.0`wl_event_loop_dispatch + 202 libwayland-server.so.0`wl_event_loop_dispatch: -> 0x7ffff794d1ca <+202>: addq $0xc, %r15 0x7ffff794d1ce <+206>: cmpq %r15, %rbp 0x7ffff794d1d1 <+209>: jne 0x7ffff794d1b8 ; <+184> 0x7ffff794d1d3 <+211>: movq 0x8(%rsp), %rcx (lldb) up frame #9: 0x00007ffff794ad37 libwayland-server.so.0`wl_display_run + 39 libwayland-server.so.0`wl_display_run: -> 0x7ffff794ad37 <+39>: movl 0x8(%rbx), %eax 0x7ffff794ad3a <+42>: testl %eax, %eax 0x7ffff794ad3c <+44>: jne 0x7ffff794ad20 ; <+16> 0x7ffff794ad3e <+46>: popq %rbx (lldb) up frame #10: 0x000055555557689a sway`server_run(server=0x00005555555f26c0) at server.c:307:2 304 wlr_backend_destroy(server->backend); 305 return false; 306 } -> 307 308 return true; 309 } 310 (lldb) up frame #11: 0x0000555555575a93 sway`main(argc=3, argv=0x00007fffffffe978) at main.c:431:2 428 swaynag_show(&config->swaynag_config_errors); 429 } 430 -> 431 server_run(&server); 432 433 shutdown: 434 sway_log(SWAY_INFO, "Shutting down sway"); --- types/wlr_xdg_foreign_v1.c | 7 +++++-- types/wlr_xdg_foreign_v2.c | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/types/wlr_xdg_foreign_v1.c b/types/wlr_xdg_foreign_v1.c index 49216e68..bb747517 100644 --- a/types/wlr_xdg_foreign_v1.c +++ b/types/wlr_xdg_foreign_v1.c @@ -33,7 +33,7 @@ static bool verify_is_toplevel(struct wl_resource *client_resource, if (wlr_surface_is_xdg_surface(surface)) { struct wlr_xdg_surface *xdg_surface = wlr_xdg_surface_from_wlr_surface(surface); - if (xdg_surface->role != WLR_XDG_SURFACE_ROLE_TOPLEVEL) { + if (xdg_surface == NULL || xdg_surface->role != WLR_XDG_SURFACE_ROLE_TOPLEVEL) { wl_resource_post_error(client_resource, -1, "surface must be an xdg_toplevel"); return false; @@ -151,7 +151,10 @@ static void destroy_imported(struct wlr_xdg_imported_v1 *imported) { wl_list_for_each_safe(child, child_tmp, &imported->children, link) { struct wlr_xdg_surface *xdg_child = wlr_xdg_surface_from_wlr_surface(child->surface); - wlr_xdg_toplevel_set_parent(xdg_child, NULL); + + if (xdg_child != NULL) { + wlr_xdg_toplevel_set_parent(xdg_child, NULL); + } } wl_list_remove(&imported->exported_destroyed.link); diff --git a/types/wlr_xdg_foreign_v2.c b/types/wlr_xdg_foreign_v2.c index c9ef5496..7f7bff06 100644 --- a/types/wlr_xdg_foreign_v2.c +++ b/types/wlr_xdg_foreign_v2.c @@ -42,7 +42,7 @@ static bool verify_is_toplevel(struct wl_resource *client_resource, struct wlr_xdg_surface *xdg_surface = wlr_xdg_surface_from_wlr_surface(surface); - if (xdg_surface->role != WLR_XDG_SURFACE_ROLE_TOPLEVEL) { + if (xdg_surface == NULL || xdg_surface->role != WLR_XDG_SURFACE_ROLE_TOPLEVEL) { wl_resource_post_error(client_resource, ZXDG_EXPORTER_V2_ERROR_INVALID_SURFACE, "surface must be an xdg_toplevel"); @@ -157,7 +157,10 @@ static void destroy_imported(struct wlr_xdg_imported_v2 *imported) { wl_list_for_each_safe(child, child_tmp, &imported->children, link) { struct wlr_xdg_surface *xdg_child = wlr_xdg_surface_from_wlr_surface(child->surface); - wlr_xdg_toplevel_set_parent(xdg_child, NULL); + + if (xdg_child != NULL) { + wlr_xdg_toplevel_set_parent(xdg_child, NULL); + } } wl_list_remove(&imported->exported_destroyed.link);