[Wayland-bugs] [Bug 100878] SIGSEGV on wl_list_insert

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=100878

            Bug ID: 100878
           Summary: SIGSEGV on wl_list_insert
           Product: Wayland
           Version: unspecified
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: medium
         Component: weston
          Assignee: wayland-bugs at lists.freedesktop.org
          Reporter: worknesday at gmail.com

As seen on SHA 9ad4de1f7ad411fcb5a62eb85e17cf96ae076a0f; no modification

At the bottom of the description, you can find stacktrace.

Analysis:
When I saw this issue, I did some digging around on wl_list type and I suspect
the seg fault arose from the design of the circular-doubly-linked lists
themselves.

If we look at wayland-util.h (where these list functions are declared), you can
see something odd and (potentially) dangerous:

    void
    wl_list_insert(struct wl_list *list, struct wl_list *elm);

    void
    wl_list_remove(struct wl_list *elm); // 

The problem here is that the *identity* of the list is subject to change as you
add/remove nodes -- especially wl_list_remove. If, say, you have a list

    struct wl_list *my_list; // initialized to some list with 3 elements

then you do

    wl_list_remove(my_list)

then you will lose the other elements and my_list is no longer valid nor
circular because wl_list_remove sets prev and next to NULL. Indeed,
wl_list_remove says so explicitly:

    /**
     * Removes an element from the list.
     *
     * \note This operation leaves \p elm in an invalid state.
     *
     * \param elm Link of the containing struct to remove from the list
     *
     * \memberof wl_list
     */

I think a better (robust) design is to wrap and hide the actual node pointers
so that
    1. we preserve the identity (pointer) of the list itself
    2. we can call wl_list_remove multiple times

I envision something like

    struct wl_list_node {
            /** Previous list element */
            struct wl_list_node *prev;
            /** Next list element */
            struct wl_list_node *next;
    };

and wl_list will be

    struct wl_list {
            struct wl_list_node *node;
    };

Then, removing a node would be something like

    // assert that node is an element of list
    void
    wl_list_remove(struct wl_list *list, struct wl_list_node *node);

So, you can snip node out of list. If list->node == node, then you can perform
a fixup (e.g. nudge list->node to the "next" element).

Thus, you ensure the integrity of the list.

I think this is the best option given the circumstance and I hope that I'm not
stepping on peoples' toes.


Stack trace:
    #0  0x00007ffff79a0313 in wl_list_insert () from
/usr/lib64/libwayland-server.so.0
    #1  0x00007fffeeae16cf in wl_signal_add (signal=<optimized out>,
listener=<optimized out>) at /usr/include/wayland-server-core.h:320
    #2  focus_state_set_focus (state=<optimized out>, surface=<optimized out>)
at desktop-shell/shell.c:735
    #3  0x00007fffeeae7435 in activate (shell=0x7820e0, view=<optimized out>,
seat=0x622100, flags=3) at desktop-shell/shell.c:3686
    #4  0x00007ffff7bc5d5d in weston_compositor_run_button_binding
(compositor=compositor at entry=0x614c60, pointer=pointer at entry=0x76c900, 
        time=time at entry=3071980015, button=button at entry=272,
state=state at entry=WL_POINTER_BUTTON_STATE_PRESSED) at libweston/bindings.c:368
    #5  0x00007ffff7bc13cd in notify_button (seat=seat at entry=0x622100,
time=3071980015, button=button at entry=272, 
        state=WL_POINTER_BUTTON_STATE_PRESSED) at libweston/input.c:1673
    #6  0x00007ffff531065b in x11_backend_deliver_button_event (event=0x7bc180,
b=0x622070) at libweston/compositor-x11.c:1230
    #7  x11_backend_handle_event (fd=<optimized out>, mask=<optimized out>,
data=0x622070) at libweston/compositor-x11.c:1422
    #8  0x00007ffff799dd12 in wl_event_loop_dispatch () from
/usr/lib64/libwayland-server.so.0
    #9  0x00007ffff799c1b5 in wl_display_run () from
/usr/lib64/libwayland-server.so.0
    #10 0x0000000000405719 in main (argc=1, argv=<optimized out>) at
compositor/main.c:1969

surface data:
    (gdb) p *es
    $6 = {resource = 0x7b2fb0, destroy_signal = {listener_list = {prev =
0x622190, next = 0x793a90}}, ...

    (gdb) p *es->destroy_signal.listener_list.prev
    $10 = {prev = 0x0, next = 0x7e1348}

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-bugs/attachments/20170429/4caec398/attachment.html>