present: fix freed pointer access

Submitted by Lionel Landwerlin on Aug. 24, 2018, 9:27 p.m.

Details

Message ID 20180824212749.16377-1-lionel.g.landwerlin@intel.com
State New
Headers show
Series "present: fix freed pointer access" ( rev: 1 ) in X.org

Not browsing as part of any series.

Commit Message

Lionel Landwerlin Aug. 24, 2018, 9:27 p.m.
When a vblank has been marked as aborted, it's going to be free in the
flip_notify function when stopped. We can't notify it after it's
stopped because the pointer is invalid.

Valgrind backtrace:

==5331== Invalid read of size 8
==5331==    at 0x212B4D: present_vblank_notify (present_vblank.c:34)
==5331==    by 0x21439B: present_wnmd_flip_notify (present_wnmd.c:194)
==5331==    by 0x21439B: present_wnmd_event_notify (present_wnmd.c:228)
==5331==    by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)
==5331==    by 0x6570FCD: ffi_call_unix64 (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
==5331==    by 0x657093E: ffi_call (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
==5331==    by 0x4DDB183: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331==    by 0x4DD79D8: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331==    by 0x4DD8EA3: wl_display_dispatch_queue_pending (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331==    by 0x14BCCA: xwl_read_events (xwayland.c:814)
==5331==    by 0x2AC0D0: ospoll_wait (ospoll.c:651)
==5331==    by 0x2A5322: WaitForSomething (WaitFor.c:208)
==5331==    by 0x27574B: Dispatch (dispatch.c:421)
==5331==  Address 0x1b44dc98 is 40 bytes inside a block of size 184 free'd
==5331==    at 0x48369EB: free (vg_replace_malloc.c:530)
==5331==    by 0x213B0A: present_wnmd_free_idle_vblanks (present_wnmd.c:118)
==5331==    by 0x213B0A: present_wnmd_flips_stop (present_wnmd.c:161)
==5331==    by 0x2143EF: present_wnmd_flip_notify (present_wnmd.c:192)
==5331==    by 0x2143EF: present_wnmd_event_notify (present_wnmd.c:228)
==5331==    by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)
==5331==    by 0x6570FCD: ffi_call_unix64 (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
==5331==    by 0x657093E: ffi_call (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
==5331==    by 0x4DDB183: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331==    by 0x4DD79D8: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331==    by 0x4DD8EA3: wl_display_dispatch_queue_pending (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
==5331==    by 0x14BCCA: xwl_read_events (xwayland.c:814)
==5331==    by 0x2AC0D0: ospoll_wait (ospoll.c:651)
==5331==    by 0x2A5322: WaitForSomething (WaitFor.c:208)
==5331==  Block was alloc'd at
==5331==    at 0x48377D5: calloc (vg_replace_malloc.c:711)
==5331==    by 0x212D9F: present_vblank_create (present_vblank.c:69)
==5331==    by 0x214014: present_wnmd_pixmap (present_wnmd.c:610)
==5331==    by 0x21576C: proc_present_pixmap (present_request.c:150)
==5331==    by 0x27599D: Dispatch (dispatch.c:479)
==5331==    by 0x279945: dix_main (main.c:276)
==5331==    by 0x633AB16: (below main) (libc-start.c:310)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107314
---
 present/present_wnmd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/present/present_wnmd.c b/present/present_wnmd.c
index 80ffb014e..48f66b357 100644
--- a/present/present_wnmd.c
+++ b/present/present_wnmd.c
@@ -190,8 +190,9 @@  present_wnmd_flip_notify(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_
 
     if (vblank->abort_flip)
         present_wnmd_flips_stop(window);
+    else
+        present_vblank_notify(vblank, PresentCompleteKindPixmap, PresentCompleteModeFlip, ust, crtc_msc);
 
-    present_vblank_notify(vblank, PresentCompleteKindPixmap, PresentCompleteModeFlip, ust, crtc_msc);
     present_wnmd_flip_try_ready(window);
 }
 

Comments

I've noticed at least another bug report which seems to abort on 
corrupted memory.
This could be the source of it (I'm still running the Xwayland that 
produced this backtrace... YOLO).

-
Lionel

On 24/08/2018 22:27, Lionel Landwerlin wrote:
> When a vblank has been marked as aborted, it's going to be free in the
> flip_notify function when stopped. We can't notify it after it's
> stopped because the pointer is invalid.
>
> Valgrind backtrace:
>
> ==5331== Invalid read of size 8
> ==5331==    at 0x212B4D: present_vblank_notify (present_vblank.c:34)
> ==5331==    by 0x21439B: present_wnmd_flip_notify (present_wnmd.c:194)
> ==5331==    by 0x21439B: present_wnmd_event_notify (present_wnmd.c:228)
> ==5331==    by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)
> ==5331==    by 0x6570FCD: ffi_call_unix64 (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
> ==5331==    by 0x657093E: ffi_call (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
> ==5331==    by 0x4DDB183: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x4DD79D8: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x4DD8EA3: wl_display_dispatch_queue_pending (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x14BCCA: xwl_read_events (xwayland.c:814)
> ==5331==    by 0x2AC0D0: ospoll_wait (ospoll.c:651)
> ==5331==    by 0x2A5322: WaitForSomething (WaitFor.c:208)
> ==5331==    by 0x27574B: Dispatch (dispatch.c:421)
> ==5331==  Address 0x1b44dc98 is 40 bytes inside a block of size 184 free'd
> ==5331==    at 0x48369EB: free (vg_replace_malloc.c:530)
> ==5331==    by 0x213B0A: present_wnmd_free_idle_vblanks (present_wnmd.c:118)
> ==5331==    by 0x213B0A: present_wnmd_flips_stop (present_wnmd.c:161)
> ==5331==    by 0x2143EF: present_wnmd_flip_notify (present_wnmd.c:192)
> ==5331==    by 0x2143EF: present_wnmd_event_notify (present_wnmd.c:228)
> ==5331==    by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)
> ==5331==    by 0x6570FCD: ffi_call_unix64 (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
> ==5331==    by 0x657093E: ffi_call (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
> ==5331==    by 0x4DDB183: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x4DD79D8: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x4DD8EA3: wl_display_dispatch_queue_pending (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x14BCCA: xwl_read_events (xwayland.c:814)
> ==5331==    by 0x2AC0D0: ospoll_wait (ospoll.c:651)
> ==5331==    by 0x2A5322: WaitForSomething (WaitFor.c:208)
> ==5331==  Block was alloc'd at
> ==5331==    at 0x48377D5: calloc (vg_replace_malloc.c:711)
> ==5331==    by 0x212D9F: present_vblank_create (present_vblank.c:69)
> ==5331==    by 0x214014: present_wnmd_pixmap (present_wnmd.c:610)
> ==5331==    by 0x21576C: proc_present_pixmap (present_request.c:150)
> ==5331==    by 0x27599D: Dispatch (dispatch.c:479)
> ==5331==    by 0x279945: dix_main (main.c:276)
> ==5331==    by 0x633AB16: (below main) (libc-start.c:310)
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Reviewed-by: Daniel Stone <daniels@collabora.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107314
> ---
>   present/present_wnmd.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/present/present_wnmd.c b/present/present_wnmd.c
> index 80ffb014e..48f66b357 100644
> --- a/present/present_wnmd.c
> +++ b/present/present_wnmd.c
> @@ -190,8 +190,9 @@ present_wnmd_flip_notify(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_
>   
>       if (vblank->abort_flip)
>           present_wnmd_flips_stop(window);
> +    else
> +        present_vblank_notify(vblank, PresentCompleteKindPixmap, PresentCompleteModeFlip, ust, crtc_msc);
>   
> -    present_vblank_notify(vblank, PresentCompleteKindPixmap, PresentCompleteModeFlip, ust, crtc_msc);
>       present_wnmd_flip_try_ready(window);
>   }
>
The Valgrind backtrace is very similar to the gdb backtraces in:
https://bugs.freedesktop.org/show_bug.cgi?id=106620

The patch could be a fix for this bug as well, which would be awesome.
On 25/08/2018 17:50, Roman Gilg wrote:
> The Valgrind backtrace is very similar to the gdb backtraces in:
> https://bugs.freedesktop.org/show_bug.cgi?id=106620
>
> The patch could be a fix for this bug as well, which would be awesome.

Yeah, I believe that's the same bug.

We should probably mark that in bugzilla or update the patch.


-

Lionel
Looking at the code I think it would be better to not use an else, but put
the present_vblank_notify call before the if (vblank->abort_flip) line.
Even if the flip was aborted, the client should receive a complete event.
At least that's what we do in case of screen flips.

On Fri, Aug 24, 2018 at 11:31 PM Lionel Landwerlin <
lionel.g.landwerlin@intel.com> wrote:

> When a vblank has been marked as aborted, it's going to be free in the
> flip_notify function when stopped. We can't notify it after it's
> stopped because the pointer is invalid.
>
> Valgrind backtrace:
>
> ==5331== Invalid read of size 8
> ==5331==    at 0x212B4D: present_vblank_notify (present_vblank.c:34)
> ==5331==    by 0x21439B: present_wnmd_flip_notify (present_wnmd.c:194)
> ==5331==    by 0x21439B: present_wnmd_event_notify (present_wnmd.c:228)
> ==5331==    by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)
> ==5331==    by 0x6570FCD: ffi_call_unix64 (in
> /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
> ==5331==    by 0x657093E: ffi_call (in
> /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
> ==5331==    by 0x4DDB183: ??? (in
> /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x4DD79D8: ??? (in
> /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x4DD8EA3: wl_display_dispatch_queue_pending (in
> /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x14BCCA: xwl_read_events (xwayland.c:814)
> ==5331==    by 0x2AC0D0: ospoll_wait (ospoll.c:651)
> ==5331==    by 0x2A5322: WaitForSomething (WaitFor.c:208)
> ==5331==    by 0x27574B: Dispatch (dispatch.c:421)
> ==5331==  Address 0x1b44dc98 is 40 bytes inside a block of size 184 free'd
> ==5331==    at 0x48369EB: free (vg_replace_malloc.c:530)
> ==5331==    by 0x213B0A: present_wnmd_free_idle_vblanks
> (present_wnmd.c:118)
> ==5331==    by 0x213B0A: present_wnmd_flips_stop (present_wnmd.c:161)
> ==5331==    by 0x2143EF: present_wnmd_flip_notify (present_wnmd.c:192)
> ==5331==    by 0x2143EF: present_wnmd_event_notify (present_wnmd.c:228)
> ==5331==    by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)
> ==5331==    by 0x6570FCD: ffi_call_unix64 (in
> /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
> ==5331==    by 0x657093E: ffi_call (in
> /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
> ==5331==    by 0x4DDB183: ??? (in
> /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x4DD79D8: ??? (in
> /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x4DD8EA3: wl_display_dispatch_queue_pending (in
> /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)
> ==5331==    by 0x14BCCA: xwl_read_events (xwayland.c:814)
> ==5331==    by 0x2AC0D0: ospoll_wait (ospoll.c:651)
> ==5331==    by 0x2A5322: WaitForSomething (WaitFor.c:208)
> ==5331==  Block was alloc'd at
> ==5331==    at 0x48377D5: calloc (vg_replace_malloc.c:711)
> ==5331==    by 0x212D9F: present_vblank_create (present_vblank.c:69)
> ==5331==    by 0x214014: present_wnmd_pixmap (present_wnmd.c:610)
> ==5331==    by 0x21576C: proc_present_pixmap (present_request.c:150)
> ==5331==    by 0x27599D: Dispatch (dispatch.c:479)
> ==5331==    by 0x279945: dix_main (main.c:276)
> ==5331==    by 0x633AB16: (below main) (libc-start.c:310)
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Reviewed-by: Daniel Stone <daniels@collabora.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107314
> ---
>  present/present_wnmd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/present/present_wnmd.c b/present/present_wnmd.c
> index 80ffb014e..48f66b357 100644
> --- a/present/present_wnmd.c
> +++ b/present/present_wnmd.c
> @@ -190,8 +190,9 @@ present_wnmd_flip_notify(present_vblank_ptr vblank,
> uint64_t ust, uint64_t crtc_
>
>      if (vblank->abort_flip)
>          present_wnmd_flips_stop(window);
> +    else
> +        present_vblank_notify(vblank, PresentCompleteKindPixmap,
> PresentCompleteModeFlip, ust, crtc_msc);
>
> -    present_vblank_notify(vblank, PresentCompleteKindPixmap,
> PresentCompleteModeFlip, ust, crtc_msc);
>      present_wnmd_flip_try_ready(window);
>  }
>
> --
> 2.18.0
>
> _______________________________________________
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel