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

bugfix: add null check on output gbm on pageflip #34

Merged
merged 1 commit into from
Jul 25, 2017
Merged

bugfix: add null check on output gbm on pageflip #34

merged 1 commit into from
Jul 25, 2017

Conversation

acrisci
Copy link

@acrisci acrisci commented Jul 24, 2017

The gbm for the output might be null for the pageflip in the case that
the output has been disconnected. The gbm might be set to null by
wlr_drm_output_cleanup() in this case.

If the output is cleaned up before the pageflip, then a double free
will crash the compositor on the call to gbm_surface_release_buffer()
in the pageflip handler. The outputs buffer object bo[1] will point to
invalid memory.

bugfix: add null check on output gbm on pageflip
8b6dc48 
The gbm for the output might be null for the pageflip in the case that
the output has been disconnected. The gbm might be set to null by
wlr_drm_output_cleanup() in this case.

If the output is cleaned up before the pageflip, then a double free
will crash the compositor on the call to gbm_surface_release_buffer()
in the pageflip handler. The outputs buffer object bo[1] will point to
invalid memory.
@acrisci
Copy link
Author

acrisci commented Jul 25, 2017

#9 told me to check drm hotplugging, so I did and noticed that it crashed most of the time so I decided to investigate.

I don't know exactly what's going on with the code because I'm not familiar with wayland or gbm, but this seems to fix the issue.

I tried a guard on the pageflip handler to return early when the state is DRM_OUTPUT_DISCONECTED but that led to a nasty hang and I had to restart my computer, so this is the solution I landed on.

I did indeed observe the case where the output->gbm is null in the pageflip handler after the output cleanup function ran.

Here is a log (with valgrind output):

==8738== Memcheck, a memory error detector
==8738== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==8738== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==8738== Command: ./bin/simple
==8738== 
2017-07-24 17:38:00 - [session/logind.c:350] Successfully loaded logind session
2017-07-24 17:38:00 - [backend/udev.c:195] Successfully initialized udev
2017-07-24 17:38:01 - [backend/drm/backend.c:84] Initalizing DRM backend for /dev/dri/card0 (i915)
2017-07-24 17:38:01 - [backend/egl.c:150] Using EGL 1.4
2017-07-24 17:38:01 - [backend/egl.c:152] Supported EGL extensions: EGL_EXT_buffer_age EGL_EXT_image_dma_buf_import EGL_KHR_config_attribs EGL_KHR_create_context EGL_KHR_fence_sync EGL_KHR_get_all_proc_addresses EGL_KHR_gl_renderbuffer_image EGL_KHR_gl_texture_2D_image EGL_KHR_gl_texture_cubemap_image EGL_KHR_image EGL_KHR_image_base EGL_KHR_image_pixmap EGL_KHR_no_config_context EGL_KHR_reusable_sync EGL_KHR_surfaceless_context EGL_KHR_wait_sync EGL_MESA_configless_context EGL_MESA_drm_image EGL_MESA_image_dma_buf_export EGL_WL_bind_wayland_display 
2017-07-24 17:38:01 - [backend/egl.c:153] Using OpenGL ES 3.2 Mesa 17.1.5
2017-07-24 17:38:01 - [backend/egl.c:154] Supported OpenGL ES extensions: GL_EXT_blend_minmax GL_EXT_multi_draw_arrays GL_EXT_texture_filter_anisotropic GL_EXT_texture_compression_dxt1 GL_EXT_texture_format_BGRA8888 GL_OES_compressed_ETC1_RGB8_texture GL_OES_depth24 GL_OES_element_index_uint GL_OES_fbo_render_mipmap GL_OES_mapbuffer GL_OES_rgb8_rgba8 GL_OES_standard_derivatives GL_OES_stencil8 GL_OES_texture_3D GL_OES_texture_float GL_OES_texture_float_linear GL_OES_texture_half_float GL_OES_texture_half_float_linear GL_OES_texture_npot GL_OES_vertex_half_float GL_EXT_texture_sRGB_decode GL_OES_EGL_image GL_OES_depth_texture GL_OES_packed_depth_stencil GL_EXT_texture_type_2_10_10_10_REV GL_OES_get_program_binary GL_APPLE_texture_max_level GL_EXT_discard_framebuffer GL_EXT_read_format_bgra GL_EXT_frag_depth GL_NV_fbo_color_attachments GL_OES_EGL_image_external GL_OES_EGL_sync GL_OES_vertex_array_object GL_OES_viewport_array GL_ANGLE_texture_compression_dxt3 GL_ANGLE_texture_compression_dxt5 GL_EXT_robustness GL_EXT_texture_rg GL_EXT_unpack_subimage GL_NV_draw_buffers GL_NV_read_buffer GL_NV_read_depth GL_NV_read_depth_stencil GL_NV_read_stencil GL_EXT_draw_buffers GL_EXT_map_buffer_range GL_KHR_debug GL_KHR_robustness GL_KHR_texture_compression_astc_ldr GL_OES_depth_texture_cube_map GL_OES_surfaceless_context GL_EXT_color_buffer_float GL_EXT_separate_shader_objects GL_EXT_shader_framebuffer_fetch GL_EXT_shader_integer_mix GL_EXT_tessellation_point_size GL_EXT_tessellation_shader GL_INTEL_conservative_rasterization GL_INTEL_performance_query GL_ANDROID_extension_pack_es31a GL_EXT_compressed_ETC1_RGB8_sub_texture GL_EXT_copy_image GL_EXT_draw_buffers_indexed GL_EXT_draw_elements_base_vertex GL_EXT_gpu_shader5 GL_EXT_polygon_offset_clamp GL_EXT_primitive_bounding_box GL_EXT_shader_io_blocks GL_EXT_texture_border_clamp GL_EXT_texture_buffer GL_EXT_texture_cube_map_array GL_KHR_blend_equation_advanced GL_KHR_blend_equation_advanced_coherent GL_KHR_context_flush_control GL_KHR_robust_buffer_access_behavior GL_NV_image_formats GL_OES_copy_image GL_OES_draw_buffers_indexed GL_OES_draw_elements_base_vertex GL_OES_gpu_shader5 GL_OES_primitive_bounding_box GL_OES_sample_shading GL_OES_sample_variables GL_OES_shader_io_blocks GL_OES_shader_multisample_interpolation GL_OES_tessellation_point_size GL_OES_tessellation_shader GL_OES_texture_border_clamp GL_OES_texture_buffer GL_OES_texture_cube_map_array GL_OES_texture_stencil8 GL_OES_texture_storage_multisample_2d_array GL_EXT_blend_func_extended GL_EXT_buffer_storage GL_EXT_geometry_point_size GL_EXT_geometry_shader GL_EXT_shader_samples_identical GL_KHR_texture_compression_astc_sliced_3d GL_OES_geometry_point_size GL_OES_geometry_shader GL_OES_shader_image_atomic GL_EXT_clip_cull_distance GL_MESA_shader_integer_functions 
2017-07-24 17:38:01 - [examples/shared.c:488] Running compositor on wayland display 'wayland-0'
2017-07-24 17:38:01 - [backend/libinput/backend.c:46] Initializing libinput
2017-07-24 17:38:01 - [backend/libinput/backend.c:76] libinput sucessfully initialized
2017-07-24 17:38:01 - [backend/drm/drm.c:506] Scanning DRM connectors
2017-07-24 17:38:01 - [backend/drm/drm.c:583] Found display 'eDP-1'
2017-07-24 17:38:01 - [backend/drm/drm.c:593] 'eDP-1' connected
2017-07-24 17:38:01 - [backend/drm/drm.c:594] Detected modes:
2017-07-24 17:38:01 - [backend/drm/drm.c:608]   3840@2160@60000
2017-07-24 17:38:01 - [backend/drm/drm.c:608]   3840@2160@47999
2017-07-24 17:38:01 - [backend/drm/drm.c:614] Sending modesetting signal for 'eDP-1'
2017-07-24 17:38:01 - [examples/shared.c:401] Output 'eDP-1' added
2017-07-24 17:38:01 - [examples/shared.c:403] Unknown  344mm x 194mm
2017-07-24 17:38:01 - [backend/drm/drm.c:227] Modesetting 'eDP-1' with '3840x2160@60000 mHz'
2017-07-24 17:38:02 - [backend/drm/drm.c:583] Found display 'HDMI-A-1'
2017-07-24 17:38:02 - [backend/drm/drm.c:593] 'HDMI-A-1' connected
2017-07-24 17:38:02 - [backend/drm/drm.c:594] Detected modes:
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   1920@1080@59934
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   1600@1200@60000
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   1680@1050@59883
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   1280@1024@60020
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   1440@900@59901
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   1280@960@60000
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   1280@800@59910
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   1024@768@60004
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   800@600@60317
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   800@600@56250
2017-07-24 17:38:02 - [backend/drm/drm.c:608]   640@480@59940
2017-07-24 17:38:02 - [backend/drm/drm.c:614] Sending modesetting signal for 'HDMI-A-1'
2017-07-24 17:38:02 - [examples/shared.c:401] Output 'HDMI-A-1' added
2017-07-24 17:38:02 - [examples/shared.c:403] Samsung Electric Company SyncMaster 477mm x 268mm
2017-07-24 17:38:02 - [backend/drm/drm.c:227] Modesetting 'HDMI-A-1' with '1920x1080@59934 mHz'
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Power Button [0:1]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Video Bus [0:6]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Video Bus [0:6]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Power Button [0:1]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Lid Switch [0:5]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Sleep Button [0:3]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added CM Storm Coolermaster Novatouch TKL [9494:39]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added CM Storm Coolermaster Novatouch TKL [9494:39]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Razer Razer DeathAdder 2013 [5426:55]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Razer Razer DeathAdder 2013 [5426:55]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Razer Razer DeathAdder 2013 [5426:55]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Integrated_Webcam_HD [7119:11146]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added G2Touch Multi-Touch by G2TSP [10900:21217]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added ELAN1010:00 04F3:3012 Touchpad [1267:12306]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added HDA Intel PCH Headphone Mic [0:0]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added HDA Intel PCH HDMI/DP,pcm=3 [0:0]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added HDA Intel PCH HDMI/DP,pcm=7 [0:0]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added HDA Intel PCH HDMI/DP,pcm=8 [0:0]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added HDA Intel PCH HDMI/DP,pcm=9 [0:0]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added HDA Intel PCH HDMI/DP,pcm=10 [0:0]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added AT Translated Set 2 keyboard [1:1]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added ETPS/2 Elantech Touchpad [2:14]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added DELL Wireless hotkeys [0:0]
2017-07-24 17:38:03 - [backend/libinput/events.c:66] Added Dell WMI hotkeys [0:0]
2017-07-24 17:38:06 - [backend/udev.c:142] udev event for card0 (change)
2017-07-24 17:38:06 - [backend/drm/backend.c:71] /dev/dri/card0 invalidated
2017-07-24 17:38:06 - [backend/drm/drm.c:506] Scanning DRM connectors
2017-07-24 17:38:06 - [backend/drm/drm.c:619] 'HDMI-A-1' disconnected
2017-07-24 17:38:06 - [backend/drm/drm.c:697] Emmiting destruction signal for 'HDMI-A-1'
==8738== Invalid read of size 8
==8738==    at 0x528EF40: gbm_surface_release_buffer (in /usr/lib64/libgbm.so.1.0.0)
==8738==    by 0x4103EE: page_flip_handler (in /home/tony/projects/wlroots/build/bin/simple)
==8738==    by 0x508654D: drmHandleEvent (in /usr/lib64/libdrm.so.2.4.0)
==8738==    by 0x410499: wlr_drm_event (in /home/tony/projects/wlroots/build/bin/simple)
==8738==    by 0x601EC51: wl_event_loop_dispatch (in /usr/lib64/libwayland-server.so.0.1.0)
==8738==    by 0x601D499: wl_display_run (in /usr/lib64/libwayland-server.so.0.1.0)
==8738==    by 0x40AF13: compositor_run (in /home/tony/projects/wlroots/build/bin/simple)
==8738==    by 0x4095E2: main (in /home/tony/projects/wlroots/build/bin/simple)
==8738==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==8738== 
==8738== 
==8738== Process terminating with default action of signal 11 (SIGSEGV)
==8738==  Access not within mapped region at address 0x0
==8738==    at 0x528EF40: gbm_surface_release_buffer (in /usr/lib64/libgbm.so.1.0.0)
==8738==    by 0x4103EE: page_flip_handler (in /home/tony/projects/wlroots/build/bin/simple)
==8738==    by 0x508654D: drmHandleEvent (in /usr/lib64/libdrm.so.2.4.0)
==8738==    by 0x410499: wlr_drm_event (in /home/tony/projects/wlroots/build/bin/simple)
==8738==    by 0x601EC51: wl_event_loop_dispatch (in /usr/lib64/libwayland-server.so.0.1.0)
==8738==    by 0x601D499: wl_display_run (in /usr/lib64/libwayland-server.so.0.1.0)
==8738==    by 0x40AF13: compositor_run (in /home/tony/projects/wlroots/build/bin/simple)
==8738==    by 0x4095E2: main (in /home/tony/projects/wlroots/build/bin/simple)
==8738==  If you believe this happened as a result of a stack
==8738==  overflow in your program's main thread (unlikely but
==8738==  possible), you can try to increase the size of the
==8738==  main thread stack using the --main-stacksize= flag.
==8738==  The main thread stack size used in this run was 8388608.
==8738== 
==8738== HEAP SUMMARY:
==8738==     in use at exit: 3,623,266 bytes in 22,020 blocks
==8738==   total heap usage: 172,911 allocs, 150,891 frees, 15,286,383 bytes allocated
==8738== 
==8738== LEAK SUMMARY:
==8738==    definitely lost: 98,864 bytes in 9 blocks
==8738==    indirectly lost: 0 bytes in 0 blocks
==8738==      possibly lost: 2,576,729 bytes in 18,451 blocks
==8738==    still reachable: 947,673 bytes in 3,560 blocks
==8738==                       of which reachable via heuristic:
==8738==                         newarray           : 1,536 bytes in 16 blocks
==8738==         suppressed: 0 bytes in 0 blocks
==8738== Rerun with --leak-check=full to see details of leaked memory
==8738== 
==8738== For counts of detected and suppressed errors, rerun with: -v
==8738== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

@ddevault ddevault merged commit b1ec0dc into swaywm:master Jul 25, 2017
@ddevault
Copy link
Contributor

Thanks!

@acrisci acrisci deleted the bug/fix-cleanup-pageflip-race branch August 13, 2017 16:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@acrisci @ddevault