[PATCH xserver] xwayland: Avoid double free of RRCrtc and RROutput
Hans de Goede
hdegoede at redhat.com
Mon Aug 8 19:16:00 UTC 2016
Hi,
On 08-08-16 17:57, Olivier Fourdan wrote:
> At shutdown, the Xserver will free all its resources which includes the
> RRCrtc and RROutput created.
>
> Xwayland would do the same in its xwl_output_destroy() called from
> xwl_close_screen(), leading to a double free of existing RRCrtc
> RROutput:
>
> Invalid read of size 4
> at 0x4CDA10: RRCrtcDestroy (rrcrtc.c:689)
> by 0x426E75: xwl_output_destroy (xwayland-output.c:301)
> by 0x424144: xwl_close_screen (xwayland.c:117)
> by 0x460E17: CursorCloseScreen (cursor.c:187)
> by 0x4EB5A3: AnimCurCloseScreen (animcur.c:106)
> by 0x4EF431: present_close_screen (present_screen.c:64)
> by 0x556D40: dix_main (main.c:354)
> by 0x6F0D290: (below main) (in /usr/lib/libc-2.24.so)
> Address 0xbb1fc30 is 0 bytes inside a block of size 728 free'd
> at 0x4C2BDB0: free (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x4CCE5F: RRCrtcDestroyResource (rrcrtc.c:719)
> by 0x577541: doFreeResource (resource.c:895)
> by 0x5787B5: FreeClientResources (resource.c:1161)
> by 0x578862: FreeAllResources (resource.c:1176)
> by 0x556C54: dix_main (main.c:323)
> by 0x6F0D290: (below main) (in /usr/lib/libc-2.24.so)
> Block was alloc'd at
> at 0x4C2CA6A: calloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> by 0x4CC6DB: RRCrtcCreate (rrcrtc.c:76)
> by 0x426D1C: xwl_output_create (xwayland-output.c:264)
> by 0x4232EC: registry_global (xwayland.c:431)
> by 0x76CB1C7: ffi_call_unix64 (in /usr/lib/libffi.so.6.0.4)
> by 0x76CAC29: ffi_call (in /usr/lib/libffi.so.6.0.4)
> by 0x556CEFD: wl_closure_invoke (connection.c:935)
> by 0x5569CBF: dispatch_event.isra.4 (wayland-client.c:1310)
> by 0x556AF13: dispatch_queue (wayland-client.c:1456)
> by 0x556AF13: wl_display_dispatch_queue_pending
> (wayland-client.c:1698)
> by 0x556B33A: wl_display_roundtrip_queue (wayland-client.c:1121)
> by 0x42371C: xwl_screen_init (xwayland.c:631)
> by 0x552F60: AddScreen (dispatch.c:3864)
>
> And:
>
> Invalid read of size 4
> at 0x522890: RROutputDestroy (rroutput.c:348)
> by 0x42684E: xwl_output_destroy (xwayland-output.c:302)
> by 0x423CF4: xwl_close_screen (xwayland.c:118)
> by 0x4B6377: CursorCloseScreen (cursor.c:187)
> by 0x539503: AnimCurCloseScreen (animcur.c:106)
> by 0x53D081: present_close_screen (present_screen.c:64)
> by 0x43DBF0: dix_main (main.c:354)
> by 0x7068730: (below main) (libc-start.c:289)
> Address 0xc403190 is 0 bytes inside a block of size 154 free'd
> at 0x4C2CD5A: free (vg_replace_malloc.c:530)
> by 0x521DF3: RROutputDestroyResource (rroutput.c:389)
> by 0x45DA61: doFreeResource (resource.c:895)
> by 0x45ECFD: FreeClientResources (resource.c:1161)
> by 0x45EDC2: FreeAllResources (resource.c:1176)
> by 0x43DB04: dix_main (main.c:323)
> by 0x7068730: (below main) (libc-start.c:289)
> Block was alloc'd at
> at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> by 0x52206B: RROutputCreate (rroutput.c:84)
> by 0x426763: xwl_output_create (xwayland-output.c:270)
> by 0x422EDC: registry_global (xwayland.c:432)
> by 0x740FC57: ffi_call_unix64 (unix64.S:76)
> by 0x740F6B9: ffi_call (ffi64.c:525)
> by 0x5495A9D: wl_closure_invoke (connection.c:949)
> by 0x549283F: dispatch_event.isra.4 (wayland-client.c:1274)
> by 0x5493A13: dispatch_queue (wayland-client.c:1420)
> by 0x5493A13: wl_display_dispatch_queue_pending
> (wayland-client.c:1662)
> by 0x5493D2E: wl_display_roundtrip_queue (wayland-client.c:1085)
> by 0x4232EC: xwl_screen_init (xwayland.c:632)
> by 0x439F50: AddScreen (dispatch.c:3864)
>
> Split xwl_output_destroy() into xwl_output_destroy() which frees the
> wl_output and the xwl_output structure, and xwl_output_remove() which
> does the RRCrtcDestroy() and RROutputDestroy() and call the latter only
> when an output is effectively removed.
>
> An additional benefit, on top of avoiding a double free, is to avoid
> updating the screen size at shutdown.
>
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
Looks good to me:
Reviewed-by: Hans de Goede <hdegoede at redhat.com>
Regards,
Hans
> ---
> hw/xwayland/xwayland-output.c | 12 +++++++++---
> hw/xwayland/xwayland.c | 2 +-
> hw/xwayland/xwayland.h | 2 ++
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> index b66da13..38c92a6 100644
> --- a/hw/xwayland/xwayland-output.c
> +++ b/hw/xwayland/xwayland-output.c
> @@ -292,20 +292,26 @@ err:
> void
> xwl_output_destroy(struct xwl_output *xwl_output)
> {
> + wl_output_destroy(xwl_output->output);
> + free(xwl_output);
> +}
> +
> +void
> +xwl_output_remove(struct xwl_output *xwl_output)
> +{
> struct xwl_output *it;
> struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
> int width = 0, height = 0;
>
> - wl_output_destroy(xwl_output->output);
> - xorg_list_del(&xwl_output->link);
> RRCrtcDestroy(xwl_output->randr_crtc);
> RROutputDestroy(xwl_output->randr_output);
> + xorg_list_del(&xwl_output->link);
>
> xorg_list_for_each_entry(it, &xwl_screen->output_list, link)
> output_get_new_size(it, &height, &width);
> update_screen_size(xwl_output, width, height);
>
> - free(xwl_output);
> + xwl_output_destroy(xwl_output);
> }
>
> static Bool
> diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> index e9892f7..bf018be 100644
> --- a/hw/xwayland/xwayland.c
> +++ b/hw/xwayland/xwayland.c
> @@ -448,7 +448,7 @@ global_remove(void *data, struct wl_registry *registry, uint32_t name)
> xorg_list_for_each_entry_safe(xwl_output, tmp_xwl_output,
> &xwl_screen->output_list, link) {
> if (xwl_output->server_output_id == name) {
> - xwl_output_destroy(xwl_output);
> + xwl_output_remove(xwl_output);
> break;
> }
> }
> diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
> index 232d9f4..4b97a2e 100644
> --- a/hw/xwayland/xwayland.h
> +++ b/hw/xwayland/xwayland.h
> @@ -170,6 +170,8 @@ struct xwl_output *xwl_output_create(struct xwl_screen *xwl_screen,
>
> void xwl_output_destroy(struct xwl_output *xwl_output);
>
> +void xwl_output_remove(struct xwl_output *xwl_output);
> +
> RRModePtr xwayland_cvt(int HDisplay, int VDisplay,
> float VRefresh, Bool Reduced, Bool Interlaced);
>
>
More information about the xorg-devel
mailing list