use-after-free in subsurface_handle_destroy / view_child_destroy

[martinetd]

Once again unsure how to proceed here, ideally destroying the subsurface should always happen before the parent view but that apparently doesn't happen when closing firefox (doesn't happen 100% of the time but seems reliable enough with firefox. It looks like having firefox alone in a workspace is necessary for it to happen, I can't reproduce if there is another stacked window at least.)
Also once again, requires my local hack or using valgrind to notice.

==111901==ERROR: AddressSanitizer: heap-use-after-free on address 0x6150006610d0 at pc 0x0000004f924f bp 0x7ffedf543350 sp 0x7ffedf543340
WRITE of size 8 at 0x6150006610d0 thread T0
    #0 0x4f924e in wl_list_remove ../common/list.c:181
    #1 0x4e66c2 in view_child_destroy ../sway/tree/view.c:925
-> wl_list_remove(&child->view_unmap.link);
    #2 0x4e575c in subsurface_handle_destroy ../sway/tree/view.c:765
    #3 0x7f53fdbf2c93 in wlr_signal_emit_safe ../util/signal.c:29
    #4 0x7f53fdbeb1ba in subsurface_destroy ../types/wlr_surface.c:550
    #5 0x7f53fdbeb7f8 in subsurface_resource_destroy ../types/wlr_surface.c:695
    #6 0x7f53fdc423de  (/lib64/libwayland-server.so.0+0x83de)
    #7 0x7f53fdc42b33 in wl_resource_destroy (/lib64/libwayland-server.so.0+0x8b33)
    #8 0x7f53fdbeb817 in subsurface_handle_destroy ../types/wlr_surface.c:700
    #9 0x7f53fd2adaef in ffi_call_unix64 (/lib64/libffi.so.6+0x6aef)
    #10 0x7f53fd2ad2aa in ffi_call (/lib64/libffi.so.6+0x62aa)
    #11 0x7f53fdc47cd1  (/lib64/libwayland-server.so.0+0xdcd1)
    #12 0x7f53fdc43131  (/lib64/libwayland-server.so.0+0x9131)
    #13 0x7f53fdc45be9 in wl_event_loop_dispatch (/lib64/libwayland-server.so.0+0xbbe9)
    #14 0x7f53fdc43774 in wl_display_run (/lib64/libwayland-server.so.0+0x9774)
    #15 0x42f5f5 in server_run ../sway/server.c:216
    #16 0x42dd22 in main ../sway/main.c:409
    #17 0x7f53fd948041 in __libc_start_main (/lib64/libc.so.6+0x27041)
    #18 0x40e3bd in _start (/opt/wayland/bin/sway+0x40e3bd)

0x6150006610d0 is located 208 bytes inside of 512-byte region [0x615000661000,0x615000661200)
freed by thread T0 here:
    #0 0x7f53fe2fa317 in __interceptor_free (/lib64/libasan.so.6+0xb0317)
    #1 0x450726 in destroy ../sway/desktop/xdg_shell.c:253
    #2 0x4dfca3 in view_destroy ../sway/tree/view.c:60
    #3 0x4dfda8 in view_begin_destroy ../sway/tree/view.c:73
    #4 0x451e99 in handle_destroy ../sway/desktop/xdg_shell.c:481
    #5 0x7f53fdbf2c93 in wlr_signal_emit_safe ../util/signal.c:29
    #6 0x7f53fdbca2ef in reset_xdg_surface ../types/xdg_shell/wlr_xdg_surface.c:474
    #7 0x7f53fdbcc097 in destroy_xdg_toplevel ../types/xdg_shell/wlr_xdg_toplevel.c:559
    #8 0x7f53fdbcbdee in xdg_toplevel_handle_resource_destroy ../types/xdg_shell/wlr_xdg_toplevel.c:498
    #9 0x7f53fdc423de  (/lib64/libwayland-server.so.0+0x83de)

previously allocated by thread T0 here:
    #0 0x7f53fe2fa847 in __interceptor_calloc (/lib64/libasan.so.6+0xb0847)
    #1 0x452022 in handle_xdg_shell_surface ../sway/desktop/xdg_shell.c:502
    #2 0x7f53fdbf2c93 in wlr_signal_emit_safe ../util/signal.c:29
    #3 0x7f53fdbc9ecb in handle_xdg_surface_commit ../types/xdg_shell/wlr_xdg_surface.c:371
    #4 0x7f53fdbeab5e in surface_commit_pending ../types/wlr_surface.c:376
    #5 0x7f53fdbeaddc in surface_commit ../types/wlr_surface.c:448
    #6 0x7f53fd2adaef in ffi_call_unix64 (/lib64/libffi.so.6+0x6aef)

SUMMARY: AddressSanitizer: heap-use-after-free ../common/list.c:181 in wl_list_remove
Shadow bytes around the buggy address:
  0x0c2a800c41c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a800c41d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a800c41e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a800c41f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a800c4200: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c2a800c4210: fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd
  0x0c2a800c4220: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a800c4230: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a800c4240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a800c4250: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a800c4260: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==111901==ABORTING

[emersion]

I think view_destroy should free all the view children.

[emersion]

Note, subsurfaces are tied to the wl_surface rather than the xdg_toplevel, so destroying the toplevel before the subsurfaces is valid from a protocol POV.