Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Nuke wlr_list #3002

Merged
merged 5 commits into from
Jul 1, 2021
Merged

Nuke wlr_list #3002

merged 5 commits into from
Jul 1, 2021

Conversation

emersion
Copy link
Member

@emersion emersion commented Jul 1, 2021 •
edited
Loading

This data structure is mis-named, as it can easily be confused with wl_list. Instead, use libwayland's wl_array.

Breaking change: wlr_tablet.paths and wlr_tablet_pad.paths are now wl_array instead of wlr_list.

@emersion emersion added the breaking Breaking change in public API label Jul 1, 2021
@emersion emersion mentioned this pull request Jul 1, 2021
Merged
ifreund
ifreund approved these changes Jul 1, 2021
View reviewed changes
Copy link
Member

@ifreund ifreund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a good read and didn't find any typos or pointer screwups. There are a few places where allocation failure isn't handled (e.g. in tablet_pad.c) but those weren't handled before this patch either.

Getting rid of wlr_list is great!

@bl4ckb0ne
Copy link
Contributor

LGTM thanks!

@bl4ckb0ne bl4ckb0ne merged commit 57b70a4 into swaywm:master Jul 1, 2021
@emersion emersion mentioned this pull request Jul 1, 2021
Open
@emersion emersion deleted the nuke-wlr-list branch July 1, 2021 15:20
@mozartilize mozartilize mentioned this pull request Jul 1, 2021
Closed
emersion added a commit to emersion/wlroots that referenced this pull request Jul 8, 2021
@emersion
Fix invalid uses of wl_array_for_each
6e2a1b4 
[1] and [2] have introduced new wl_array usage in wlroots, but
contains a mistake: wl_array_for_each iterates over pointers to
the wl_array entries, not over entries themselves.

Fix all wl_array_for_each call sites. Name the variables "ptr"
to avoid confusion.

Found via ASan:

    ==148752==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x602000214111 in thread T0
        #0 0x7f6ff2235f19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
        #1 0x7f6ff1c04004 in wlr_tablet_destroy ../subprojects/wlroots/types/wlr_tablet_tool.c:24
        #2 0x7f6ff1b8463c in wlr_input_device_destroy ../subprojects/wlroots/types/wlr_input_device.c:51
        swaywm#3 0x7f6ff1ab9941 in backend_destroy ../subprojects/wlroots/backend/wayland/backend.c:306
        swaywm#4 0x7f6ff1a68323 in wlr_backend_destroy ../subprojects/wlroots/backend/backend.c:57
        swaywm#5 0x7f6ff1ab36b4 in multi_backend_destroy ../subprojects/wlroots/backend/multi/backend.c:57
        swaywm#6 0x7f6ff1ab417c in handle_display_destroy ../subprojects/wlroots/backend/multi/backend.c:124
        swaywm#7 0x7f6ff106184e in wl_display_destroy (/usr/lib/libwayland-server.so.0+0x884e)
        swaywm#8 0x55cd1a77c9e5 in server_fini ../sway/server.c:218
        swaywm#9 0x55cd1a77893f in main ../sway/main.c:400
        swaywm#10 0x7f6ff04bdb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
        swaywm#11 0x55cd1a73a7ad in _start (/home/simon/src/sway/build/sway/sway+0x33a7ad)

    0x602000214111 is located 1 bytes inside of 16-byte region [0x602000214110,0x602000214120)
    freed by thread T0 here:
        #0 0x7f6ff2235f19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
        #1 0x7f6ff1c04004 in wlr_tablet_destroy ../subprojects/wlroots/types/wlr_tablet_tool.c:24
        #2 0x7f6ff1b8463c in wlr_input_device_destroy ../subprojects/wlroots/types/wlr_input_device.c:51
        swaywm#3 0x7f6ff1ab9941 in backend_destroy ../subprojects/wlroots/backend/wayland/backend.c:306
        swaywm#4 0x7f6ff1a68323 in wlr_backend_destroy ../subprojects/wlroots/backend/backend.c:57
        swaywm#5 0x7f6ff1ab36b4 in multi_backend_destroy ../subprojects/wlroots/backend/multi/backend.c:57
        swaywm#6 0x7f6ff1ab417c in handle_display_destroy ../subprojects/wlroots/backend/multi/backend.c:124
        swaywm#7 0x7f6ff106184e in wl_display_destroy (/usr/lib/libwayland-server.so.0+0x884e)

    previously allocated by thread T0 here:
        #0 0x7f6ff2236279 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
        #1 0x7f6ff1066d03 in wl_array_add (/usr/lib/libwayland-server.so.0+0xdd03)

[1]: swaywm#3002
[2]: swaywm#3004
@emersion emersion mentioned this pull request Jul 8, 2021
Merged
bl4ckb0ne pushed a commit that referenced this pull request Jul 8, 2021
@emersion @bl4ckb0ne
Fix invalid uses of wl_array_for_each
e035f2b 
[1] and [2] have introduced new wl_array usage in wlroots, but
contains a mistake: wl_array_for_each iterates over pointers to
the wl_array entries, not over entries themselves.

Fix all wl_array_for_each call sites. Name the variables "ptr"
to avoid confusion.

Found via ASan:

    ==148752==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x602000214111 in thread T0
        #0 0x7f6ff2235f19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
        #1 0x7f6ff1c04004 in wlr_tablet_destroy ../subprojects/wlroots/types/wlr_tablet_tool.c:24
        #2 0x7f6ff1b8463c in wlr_input_device_destroy ../subprojects/wlroots/types/wlr_input_device.c:51
        #3 0x7f6ff1ab9941 in backend_destroy ../subprojects/wlroots/backend/wayland/backend.c:306
        #4 0x7f6ff1a68323 in wlr_backend_destroy ../subprojects/wlroots/backend/backend.c:57
        #5 0x7f6ff1ab36b4 in multi_backend_destroy ../subprojects/wlroots/backend/multi/backend.c:57
        #6 0x7f6ff1ab417c in handle_display_destroy ../subprojects/wlroots/backend/multi/backend.c:124
        #7 0x7f6ff106184e in wl_display_destroy (/usr/lib/libwayland-server.so.0+0x884e)
        #8 0x55cd1a77c9e5 in server_fini ../sway/server.c:218
        #9 0x55cd1a77893f in main ../sway/main.c:400
        #10 0x7f6ff04bdb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
        #11 0x55cd1a73a7ad in _start (/home/simon/src/sway/build/sway/sway+0x33a7ad)

    0x602000214111 is located 1 bytes inside of 16-byte region [0x602000214110,0x602000214120)
    freed by thread T0 here:
        #0 0x7f6ff2235f19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
        #1 0x7f6ff1c04004 in wlr_tablet_destroy ../subprojects/wlroots/types/wlr_tablet_tool.c:24
        #2 0x7f6ff1b8463c in wlr_input_device_destroy ../subprojects/wlroots/types/wlr_input_device.c:51
        #3 0x7f6ff1ab9941 in backend_destroy ../subprojects/wlroots/backend/wayland/backend.c:306
        #4 0x7f6ff1a68323 in wlr_backend_destroy ../subprojects/wlroots/backend/backend.c:57
        #5 0x7f6ff1ab36b4 in multi_backend_destroy ../subprojects/wlroots/backend/multi/backend.c:57
        #6 0x7f6ff1ab417c in handle_display_destroy ../subprojects/wlroots/backend/multi/backend.c:124
        #7 0x7f6ff106184e in wl_display_destroy (/usr/lib/libwayland-server.so.0+0x884e)

    previously allocated by thread T0 here:
        #0 0x7f6ff2236279 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
        #1 0x7f6ff1066d03 in wl_array_add (/usr/lib/libwayland-server.so.0+0xdd03)

[1]: #3002
[2]: #3004
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ifreund ifreund ifreund approved these changes

@bl4ckb0ne bl4ckb0ne bl4ckb0ne approved these changes

Assignees
No one assigned
Labels
breaking Breaking change in public API
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emersion @bl4ckb0ne @ifreund