Bug 67231

Summary: weston_release_seat() double frees focus_state
Product: Wayland Reporter: Hardening <rdp.effort>
Component: westonAssignee: Wayland bug list <wayland-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: major    
Priority: medium    
Version: unspecified   
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: GDB script for printing arguments of rdp_peer_context_free

Description Hardening 2013-07-23 21:54:12 UTC
I can reproduce this valgrind error:

using config in current working directory: ./weston.ini
using config in current working directory: ./weston.ini
using config in current working directory: ./weston.ini
[23:30:32.733] unable to checkDescriptor for 0x8812c30
==4837== Invalid write of size 8
==4837==    at 0x4E3DE77: wl_list_remove (wayland-util.c:53)
==4837==    by 0xC8CA0CC: focus_state_destroy (shell.c:421)
==4837==    by 0x4102EB: weston_seat_release (wayland-server.h:171)
==4837==    by 0x884587E: rdp_peer_context_free (compositor-rdp.c:603)
==4837==    by 0x8CF0F69: freerdp_peer_context_free (peer.c:405)
==4837==    by 0x8845BD7: rdp_client_activity (compositor-rdp.c:622)
==4837==    by 0x4E3C1A2: wl_event_loop_dispatch (event-loop.c:421)
==4837==    by 0x4E3A8F4: wl_display_run (wayland-server.c:836)
==4837==    by 0x4083C6: main (compositor.c:3398)
==4837==  Address 0xccfc6a8 is 24 bytes inside a block of size 120 free'd
==4837==    at 0x4C2BA6C: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4837==    by 0x4102EB: weston_seat_release (wayland-server.h:171)
==4837==    by 0x884587E: rdp_peer_context_free (compositor-rdp.c:603)
==4837==    by 0x8CF0F69: freerdp_peer_context_free (peer.c:405)
==4837==    by 0x8845BD7: rdp_client_activity (compositor-rdp.c:622)
==4837==    by 0x4E3C1A2: wl_event_loop_dispatch (event-loop.c:421)
==4837==    by 0x4E3A8F4: wl_display_run (wayland-server.c:836)
==4837==    by 0x4083C6: main (compositor.c:3398)
==4837== 


with this simple actions:
* launch the RDP compositor, (valgrind output/src/weston --backend=rdp-backend.so --width=800 --height=600)
* connect with xfreerdp client (xfreerdp /v:127.0.0.1)
* open 2 weston-terminal, click on the first one to grab the focus
* kill your xfreerdp, and you have the above backtrace

It matches the second backtrace of #65913, i think we were too fast to close it.
Comment 1 Mariusz Ceier 2013-07-23 23:38:32 UTC
I can confirm this, here's my stacktrace from valgrind:

==10637== Invalid write of size 8
==10637==    at 0x4E3FFD4: wl_list_remove (wayland-util.c:53)
==10637==    by 0xCEB6926: focus_state_destroy (shell.c:421)
==10637==    by 0xCEB6984: focus_state_seat_destroy (shell.c:434)
==10637==    by 0x41047C: wl_signal_emit (wayland-server.h:171)
==10637==    by 0x413BB9: weston_seat_release (input.c:1573)
==10637==    by 0x88343B3: rdp_peer_context_free (compositor-rdp.c:603)
==10637==    by 0x883445D: rdp_client_activity (compositor-rdp.c:622)
==10637==    by 0x4E3C967: wl_event_source_fd_dispatch (event-loop.c:86)
==10637==    by 0x4E3D318: wl_event_loop_dispatch (event-loop.c:421)
==10637==    by 0x4E3B615: wl_display_run (wayland-server.c:836)
==10637==    by 0x410340: main (compositor.c:3373)
==10637==  Address 0x8825968 is 24 bytes inside a block of size 120 free'd
==10637==    at 0x4C2AF7C: free (vg_replace_malloc.c:446)
==10637==    by 0x419EC5: input_method_notifier_destroy (text-backend.c:796)
==10637==    by 0x41047C: wl_signal_emit (wayland-server.h:171)
==10637==    by 0x413BB9: weston_seat_release (input.c:1573)
==10637==    by 0x88343B3: rdp_peer_context_free (compositor-rdp.c:603)
==10637==    by 0x883445D: rdp_client_activity (compositor-rdp.c:622)
==10637==    by 0x4E3C967: wl_event_source_fd_dispatch (event-loop.c:86)
==10637==    by 0x4E3D318: wl_event_loop_dispatch (event-loop.c:421)
==10637==    by 0x4E3B615: wl_display_run (wayland-server.c:836)
==10637==    by 0x410340: main (compositor.c:3373)
==10637==

I compile weston and wayland with -O0 so it includes also inline functions.
Comment 2 Rob Bradford 2013-07-24 14:23:52 UTC
Use gdb or printing the address of context can you see if rdp_peer_context_free is being called on the same context twice?

That would explain this bug. One thing I noticed is that that you add the context to the
Comment 3 Rob Bradford 2013-07-24 15:59:51 UTC
I posted two patches to the list to address this bug: the destroy signal list was corrupted by the freeing of memory containing the link nodes without removing the node from the list:

commit fad549ce2c221fccb04767e3b2f886c47dad1590
Author: Rob Bradford <rob@linux.intel.com>
Date:   Wed Jul 24 16:54:35 2013 +0100

    clipboard: remove the weston_seat destruction listener on destroy
    
    Prior to freeing the memory in which the link node for the signal is
    emedded we should remove the link node from the list to prevent the list
    from being corrupted.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=67231

commit 6e5a5caee3eec8c8ed4eeedccd27f88bf203a689
Author: Rob Bradford <rob@linux.intel.com>
Date:   Wed Jul 24 16:54:35 2013 +0100

    text-backend: remove the weston_seat destruction listener on destroy
    
    Prior to freeing the memory in which the link node for the signal is
    emedded we should remove the link node from the list to prevent the list
    from being corrupted.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=67231
Comment 4 Mariusz Ceier 2013-07-24 16:57:38 UTC
Created attachment 82945 [details]
GDB script for printing arguments of rdp_peer_context_free

Applied both patches, and they didn't solve the problem.
rdp_peer_context_free is called only once, below are the values of arguments passed, and contents of context:

client = 0x6435c0 context = 0x693970

$1 = {_p = {instance = 0x0, peer = 0x6435c0, paddingA = {0 <repeats 14 times>}, argc = 0, argv = 0x0, pubSub = 0x0, paddingB = {
      0 <repeats 13 times>}, rdp = 0x6436e0, gdi = 0x0, rail = 0x0, cache = 0x0, channels = 0x0, graphics = 0x0, input = 0x65be40, 
    update = 0x65bf00, settings = 0x649920, paddingC = {0 <repeats 23 times>}, paddingD = {0 <repeats 32 times>}, paddingE = {
      0 <repeats 32 times>}}, rdpCompositor = 0x6404a0, events = {0x6a9710, 0x0 <repeats 31 times>}, rfx_context = 0x694010, 
  encode_stream = 0x6953b0, rfx_rects = 0x0, nsc_context = 0x6952e0, item = {flags = 3, peer = 0x6435c0, seat = {
      base_resource_list = {prev = 0x6ad910, next = 0x71fc30}, global = 0x6a5400, pointer = 0x6bc360, keyboard = 0x6af6a0, 
      touch = 0x0, output = 0x0, destroy_signal = {listener_list = {prev = 0x6db2a8, next = 0x6a5480}}, compositor = 0x6404a0, 
      link = {prev = 0x640600, next = 0x640600}, modifier_state = (unknown: 0), saved_kbd_focus = 0x0, saved_kbd_focus_listener = {
        link = {prev = 0x0, next = 0x0}, notify = 0x0}, drag_resource_list = {prev = 0x6ad970, next = 0x71fc90}, 
      selection_serial = 0, selection_data_source = 0x0, selection_data_source_listener = {link = {prev = 0x0, next = 0x0}, 
        notify = 0x0}, selection_signal = {listener_list = {prev = 0x6a5468, next = 0x6a5468}}, num_tp = 0, led_update = 0x0, 
      xkb_info = {keymap = 0x6ae570, keymap_fd = 36, keymap_size = 45099, 
        keymap_area = 0x7ffff7fb4000 "xkb_keymap {\nxkb_keycodes \"evdev+aliases(qwerty)\" {\n\tminimum = 8;\n\tmaximum = 255;\n\t<ESC>", ' ' <repeats 16 times>, "= 9;\n\t<AE01>", ' ' <repeats 15 times>, "= 10;\n\t<AE02>", ' ' <repeats 15 times>, "= 11;\n\t<AE03>", ' ' <repeats 15 times>, "= 12;\n\t<AE04>"..., shift_mod = 0, caps_mod = 1, ctrl_mod = 2, alt_mod = 3, mod2_mod = 4, mod3_mod = 5, 
        super_mod = 6, mod5_mod = 7, num_led = 1, caps_led = 0, scroll_led = 2}, xkb_state = {state = 0x6ca580, 
        leds = (unknown: 0)}, input_method = 0x6a54b0, seat_name = 0x6a5440 "rdp:25:127.0.0.1"}, link = {prev = 0x641e90, 
      next = 0x641e90}}}


Attached gdb script prints these values when rdp_peer_context_free is called ("gdb -x gdbscript" to use it).

valgrind backtrace with applied patches:

==7008== Invalid read of size 8
==7008==    at 0x419D5D: unbind_input_method (text-backend.c:743)
==7008==    by 0x4E3AA8D: destroy_resource (wayland-server.c:434)
==7008==    by 0x4E407B9: for_each_helper (wayland-util.c:353)
==7008==    by 0x4E407F5: wl_map_for_each (wayland-util.c:359)
==7008==    by 0x4E3ADA5: wl_client_destroy (wayland-server.c:574)
==7008==    by 0x41A2BC: text_backend_notifier_destroy (text-backend.c:931)
==7008==    by 0x408EA8: wl_signal_emit (wayland-server.h:171)
==7008==    by 0x410361: main (compositor.c:3379)
==7008==  Address 0x8825a30 is 112 bytes inside a block of size 120 free'd
==7008==    at 0x4C2AF7C: free (vg_replace_malloc.c:446)
==7008==    by 0x419EE5: input_method_notifier_destroy (text-backend.c:797)
==7008==    by 0x41047C: wl_signal_emit (wayland-server.h:171)
==7008==    by 0x413AE8: weston_seat_release (input.c:1552)
==7008==    by 0x88343B3: rdp_peer_context_free (compositor-rdp.c:603)
==7008==    by 0x883445D: rdp_client_activity (compositor-rdp.c:622)
==7008==    by 0x4E3C967: wl_event_source_fd_dispatch (event-loop.c:86)
==7008==    by 0x4E3D318: wl_event_loop_dispatch (event-loop.c:421)
==7008==    by 0x4E3B615: wl_display_run (wayland-server.c:836)
==7008==    by 0x410340: main (compositor.c:3373)
==7008==
==7008== Invalid write of size 8
==7008==    at 0x419D69: unbind_input_method (text-backend.c:745)
==7008==    by 0x4E3AA8D: destroy_resource (wayland-server.c:434)
==7008==    by 0x4E407B9: for_each_helper (wayland-util.c:353)
==7008==    by 0x4E407F5: wl_map_for_each (wayland-util.c:359)
==7008==    by 0x4E3ADA5: wl_client_destroy (wayland-server.c:574)
==7008==    by 0x41A2BC: text_backend_notifier_destroy (text-backend.c:931)
==7008==    by 0x408EA8: wl_signal_emit (wayland-server.h:171)
==7008==    by 0x410361: main (compositor.c:3379)
==7008==  Address 0x88259c0 is 0 bytes inside a block of size 120 free'd
==7008==    at 0x4C2AF7C: free (vg_replace_malloc.c:446)
==7008==    by 0x419EE5: input_method_notifier_destroy (text-backend.c:797)
==7008==    by 0x41047C: wl_signal_emit (wayland-server.h:171)
==7008==    by 0x413AE8: weston_seat_release (input.c:1552)
==7008==    by 0x88343B3: rdp_peer_context_free (compositor-rdp.c:603)
==7008==    by 0x883445D: rdp_client_activity (compositor-rdp.c:622)
==7008==    by 0x4E3C967: wl_event_source_fd_dispatch (event-loop.c:86)
==7008==    by 0x4E3D318: wl_event_loop_dispatch (event-loop.c:421)
==7008==    by 0x4E3B615: wl_display_run (wayland-server.c:836)
==7008==    by 0x410340: main (compositor.c:3373)
==7008==
Comment 5 Rob Bradford 2013-07-25 17:59:21 UTC
Thanks for the feedback - I don't see the warnings below. They're different from the original so it looks like the original one is fixed by the first. For me that showed up the clipboard problem. I'll look for a similar problem in around the area you've highlighted in your log.
Comment 6 Hardening 2013-07-25 21:14:14 UTC
I have the impression that 9e5d7d17a79d61011a0a43db9e071bf6cf5eb500 solved the problem for me. I can't reproduce the backtrace.
Comment 7 Hardening 2013-07-25 21:15:40 UTC
(In reply to comment #6)
> I have the impression that 9e5d7d17a79d61011a0a43db9e071bf6cf5eb500 solved
> the problem for me. I can't reproduce the backtrace.

Forgot to mention i had also applied the 2 provided patches:
* fad549ce2c221fccb04767e3b2f886c47dad1590
* 6e5a5caee3eec8c8ed4eeedccd27f88bf203a689
Comment 8 Mariusz Ceier 2013-07-25 21:44:56 UTC
I did pull todays changes in weston (including 9e5d7d17a79d61011a0a43db9e071bf6cf5eb500 and Rob patches) and weston still crashes, but less often.

valgrind shows:
==6506== Invalid read of size 8
==6506==    at 0x419F89: unbind_input_method (text-backend.c:743)
==6506==    by 0x4E3AA8D: destroy_resource (wayland-server.c:434)
==6506==    by 0x4E407B9: for_each_helper (wayland-util.c:353)
==6506==    by 0x4E407F5: wl_map_for_each (wayland-util.c:359)
==6506==    by 0x4E3ADA5: wl_client_destroy (wayland-server.c:574)
==6506==    by 0x41A4E8: text_backend_notifier_destroy (text-backend.c:931)
==6506==    by 0x408EE8: wl_signal_emit (wayland-server.h:171)
==6506==    by 0x41058D: main (compositor.c:3429)
==6506==  Address 0x8825a30 is 112 bytes inside a block of size 120 free'd
==6506==    at 0x4C2AF7C: free (vg_replace_malloc.c:446)
==6506==    by 0x41A111: input_method_notifier_destroy (text-backend.c:797)
==6506==    by 0x4106A8: wl_signal_emit (wayland-server.h:171)
==6506==    by 0x413DE5: weston_seat_release (input.c:1573)
==6506==    by 0x88343B3: rdp_peer_context_free (compositor-rdp.c:603)
==6506==    by 0x883445D: rdp_client_activity (compositor-rdp.c:622)
==6506==    by 0x4E3C967: wl_event_source_fd_dispatch (event-loop.c:86)
==6506==    by 0x4E3D318: wl_event_loop_dispatch (event-loop.c:421)
==6506==    by 0x4E3B615: wl_display_run (wayland-server.c:836)
==6506==    by 0x41056C: main (compositor.c:3423)
==6506== 
==6506== Invalid write of size 8
==6506==    at 0x419F95: unbind_input_method (text-backend.c:745)
==6506==    by 0x4E3AA8D: destroy_resource (wayland-server.c:434)
==6506==    by 0x4E407B9: for_each_helper (wayland-util.c:353)
==6506==    by 0x4E407F5: wl_map_for_each (wayland-util.c:359)
==6506==    by 0x4E3ADA5: wl_client_destroy (wayland-server.c:574)
==6506==    by 0x41A4E8: text_backend_notifier_destroy (text-backend.c:931)
==6506==    by 0x408EE8: wl_signal_emit (wayland-server.h:171)
==6506==    by 0x41058D: main (compositor.c:3429)
==6506==  Address 0x88259c0 is 0 bytes inside a block of size 120 free'd
==6506==    at 0x4C2AF7C: free (vg_replace_malloc.c:446)
==6506==    by 0x41A111: input_method_notifier_destroy (text-backend.c:797)
==6506==    by 0x4106A8: wl_signal_emit (wayland-server.h:171)
==6506==    by 0x413DE5: weston_seat_release (input.c:1573)
==6506==    by 0x88343B3: rdp_peer_context_free (compositor-rdp.c:603)
==6506==    by 0x883445D: rdp_client_activity (compositor-rdp.c:622)
==6506==    by 0x4E3C967: wl_event_source_fd_dispatch (event-loop.c:86)
==6506==    by 0x4E3D318: wl_event_loop_dispatch (event-loop.c:421)
==6506==    by 0x4E3B615: wl_display_run (wayland-server.c:836)
==6506==    by 0x41056C: main (compositor.c:3423)

and gdb shows corrupted linked list.
Comment 9 Rob Bradford 2013-07-26 15:55:45 UTC
These two indicate that this is something that is happening during the weston shutdown:

==6506==    by 0x41A4E8: text_backend_notifier_destroy (text-backend.c:931)
==6506==    by 0x408EE8: wl_signal_emit (wayland-server.h:171)
==6506==    by 0x41058D: main (compositor.c:3429)

the destroy signal is only emitted as part of the shutdown. This might cause a crash when you're quitting weston but not otherwise.
Comment 10 Mariusz Ceier 2013-07-26 16:13:16 UTC
Yes, that crash is during the shutdown - but it shows up when I follow the procedure described in first comment.
Comment 11 Rob Bradford 2013-08-15 16:31:01 UTC
The original issue was fixed. But for the last different issue I opened a new bug https://bugs.freedesktop.org/show_bug.cgi?id=68157

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.