Free transaction state upon destruction by removing pointer indirection

[ianyfan]

Fixes memory leaks.

Not sure about function names, whether to check for NULL, and commit message lol

[RyanDwyer]

Wow, I can't believe I missed this.

Anyway, I don't think the logic in this PR is right. We allocate a state for every instruction, so there should be a free just as frequently. This PR only frees them when the node is destroying.

Note that when applying states we copy/re-use the list pointers, so when freeing the instruction's states we only need to free the state itself and not anything inside it.

[ianyfan]

This is more difficult than I thought it would be, so I'm not sure if this is a good solution.

Also, why did you change the state to be a pointer? Can't you have a union of structs directly?

[RyanDwyer]

Can you clarify why the server shutting down check is needed? I guess it would make the transaction complete instantly, which causes any destroying nodes to be freed before sway exits. I don't really see why that's useful.

Also, why did you change the state to be a pointer? Can't you have a union of structs directly?

I made it a union of pointers because that's what I'd seen elsewhere and didn't think too much about it. Feel free to make it a union of structs instead.

[ianyfan]

When the server shuts down, it destroys all of the wayland clients, which create transactions to destroy the sway view, as well as the rest of the nodes. If the transactions just get destroyed, it leaves these nodes hanging around as leaked memory.

If I try to force through all of the pending transactions at the end using this patch, I get this error:

2018-09-21 11:00:37 - [sway/desktop/transaction.c:284] Applying transaction 0x6040000f1fd0
2018-09-21 11:00:37 - [sway/desktop/transaction.c:404] Transaction 0x6040000f2010 committing with 2 instructions
2018-09-21 11:00:37 - [sway/desktop/transaction.c:284] Applying transaction 0x6040000f2010
2018-09-21 11:00:37 - [sway/desktop/transaction.c:404] Transaction 0x6040000f2090 committing with 2 instructions
2018-09-21 11:00:37 - [sway/desktop/transaction.c:284] Applying transaction 0x6040000f2090
=================================================================
==21859==ERROR: AddressSanitizer: heap-use-after-free on address 0x613000000168 at pc 0x7f07abeea9e0 bp 0x7ffe5064bbc0 sp 0x7ffe5064bbb0
READ of size 8 at 0x613000000168 thread T0
    #0 0x7f07abeea9df in wlr_egl_make_current ../subprojects/wlroots/render/egl.c:304
    #1 0x7f07abef1ea0 in gles2_texture_destroy ../subprojects/wlroots/render/gles2/texture.c:119
    #2 0x7f07abef48d0 in wlr_texture_destroy ../subprojects/wlroots/render/wlr_texture.c:16
    #3 0x7f07abf1f370 in wlr_buffer_unref ../subprojects/wlroots/types/wlr_buffer.c:139
    #4 0x556e016eabfa in view_remove_saved_buffer ../sway/tree/view.c:1063
    #5 0x556e0167e2c9 in apply_container_state ../sway/desktop/transaction.c:258
    #6 0x556e0167ebf0 in transaction_apply ../sway/desktop/transaction.c:312
    #7 0x556e0167f0db in transaction_progress_queue ../sway/desktop/transaction.c:349
    #8 0x556e0167f2f0 in transaction_progress_queue ../sway/desktop/transaction.c:373
    #9 0x556e0167f2f0 in transaction_progress_queue ../sway/desktop/transaction.c:373
    #10 0x556e01680a5a in destroy_transaction_queue ../sway/desktop/transaction.c:541
    #11 0x556e016668c6 in server_fini ../sway/server.c:149
    #12 0x556e016656c0 in main ../sway/main.c:451
    #13 0x7f07aaa19222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    #14 0x556e0164ac2d in _start (/usr/local/bin/sway+0x2ec2d)

0x613000000168 is located 296 bytes inside of 344-byte region [0x613000000040,0x613000000198)
freed by thread T0 here:
    #0 0x7f07ac0d3c19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:66
    #1 0x7f07abebc6b3 in backend_destroy ../subprojects/wlroots/backend/drm/backend.c:54
    #2 0x7f07abeb8f0d in wlr_backend_destroy ../subprojects/wlroots/backend/backend.c:46
    #3 0x7f07abed7afc in multi_backend_destroy ../subprojects/wlroots/backend/multi/backend.c:53
    #4 0x7f07abed7c89 in handle_display_destroy ../subprojects/wlroots/backend/multi/backend.c:84
    #5 0x7f07ab039bee  (/usr/lib/libwayland-server.so.0+0x8bee)
    #6 0x7f07ab03a376 in wl_display_destroy (/usr/lib/libwayland-server.so.0+0x9376)
    #7 0x556e01666880 in server_fini ../sway/server.c:147
    #8 0x556e016656c0 in main ../sway/main.c:451
    #9 0x7f07aaa19222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    #10 0x556e0164ac2d in _start (/usr/local/bin/sway+0x2ec2d)

previously allocated by thread T0 here:
    #0 0x7f07ac0d4231 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:95
    #1 0x7f07abebce8a in wlr_drm_backend_create ../subprojects/wlroots/backend/drm/backend.c:139
    #2 0x7f07abeb9484 in attempt_drm_backend ../subprojects/wlroots/backend/backend.c:131
    #3 0x7f07abeb9e16 in wlr_backend_autocreate ../subprojects/wlroots/backend/backend.c:265
    #4 0x556e016659f0 in server_privileged_prepare ../sway/server.c:37
    #5 0x556e016650a3 in main ../sway/main.c:364
    #6 0x7f07aaa19222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    #7 0x556e0164ac2d in _start (/usr/local/bin/sway+0x2ec2d)

SUMMARY: AddressSanitizer: heap-use-after-free ../subprojects/wlroots/render/egl.c:304 in wlr_egl_make_current
Shadow bytes around the buggy address:
  0x0c267fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c267fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c267fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c267fff8000: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c267fff8010: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c267fff8020: fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd fd
  0x0c267fff8030: fd fd fd fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c267fff8040: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c267fff8050: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c267fff8060: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
  0x0c267fff8070: fa fa fa fa fa fa fa fa 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
==21859==ABORTING

The only time everything worked correctly was when I tested it with -Dnoatomic which was where I got the commit idea from.

So honestly... I'm not really sure, I'm not grokking it currently, so I wouldn't mind if you wanted to have a go yourself.

[RyanDwyer]

When the server shuts down, it destroys all of the wayland clients, which create transactions to destroy the sway view, as well as the rest of the nodes. If the transactions just get destroyed, it leaves these nodes hanging around as leaked memory.

Where does Sway destroy the clients? I'm not seeing it.

I also don't see why this is a problem or how it's related to this PR. If I add free(instruction->output_state) as you have and don't make any other changes then it seems to work fine.

[ianyfan]

I'm happy to just add that line for now, and I'll investigate my other issue and open another PR/issue if I work out what's going on.

[RyanDwyer]

Can you please make it a union of structs rather than a union of pointers? We may as well change it.

[RyanDwyer]

I'm happy to merge this, but you still have [WIP] in the title. Please confirm if you're happy to have it merged.

[ianyfan]

Yes, go ahead

[RyanDwyer]

Thanks!