[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-xorg-devel
Subject: Re: [PATCH] present: fix freed pointer access
From: Roman Gilg <subdiff () gmail ! com>
Date: 2018-08-25 21:26:40
Message-ID: CAJcyoyv6DLHj_jnO_WxvwHrFrnsLjxFVQZhtGkvyu+2rT6g=-A () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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
[Attachment #5 (text/html)]
<div dir="ltr">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.<br></div><br><div \
class="gmail_quote"><div dir="ltr">On Fri, Aug 24, 2018 at 11:31 PM Lionel Landwerlin \
<<a href="mailto:lionel.g.landwerlin@intel.com">lionel.g.landwerlin@intel.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">When a vblank has been marked as \
aborted, it's going to be free in the<br> flip_notify function when stopped. We \
can't notify it after it's<br> stopped because the pointer is invalid.<br>
<br>
Valgrind backtrace:<br>
<br>
==5331== Invalid read of size 8<br>
==5331== at 0x212B4D: present_vblank_notify (present_vblank.c:34)<br>
==5331== by 0x21439B: present_wnmd_flip_notify (present_wnmd.c:194)<br>
==5331== by 0x21439B: present_wnmd_event_notify (present_wnmd.c:228)<br>
==5331== by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)<br>
==5331== by 0x6570FCD: ffi_call_unix64 (in \
/usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)<br> ==5331== by 0x657093E: ffi_call \
(in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)<br> ==5331== by 0x4DDB183: ??? \
(in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)<br> ==5331== by \
0x4DD79D8: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)<br> ==5331== \
by 0x4DD8EA3: wl_display_dispatch_queue_pending (in \
/usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)<br> ==5331== by 0x14BCCA: \
xwl_read_events (xwayland.c:814)<br> ==5331== by 0x2AC0D0: ospoll_wait \
(ospoll.c:651)<br> ==5331== by 0x2A5322: WaitForSomething (WaitFor.c:208)<br>
==5331== by 0x27574B: Dispatch (dispatch.c:421)<br>
==5331== Address 0x1b44dc98 is 40 bytes inside a block of size 184 free'd<br>
==5331== at 0x48369EB: free (vg_replace_malloc.c:530)<br>
==5331== by 0x213B0A: present_wnmd_free_idle_vblanks (present_wnmd.c:118)<br>
==5331== by 0x213B0A: present_wnmd_flips_stop (present_wnmd.c:161)<br>
==5331== by 0x2143EF: present_wnmd_flip_notify (present_wnmd.c:192)<br>
==5331== by 0x2143EF: present_wnmd_event_notify (present_wnmd.c:228)<br>
==5331== by 0x156216: xwl_present_sync_callback (xwayland-present.c:282)<br>
==5331== by 0x6570FCD: ffi_call_unix64 (in \
/usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)<br> ==5331== by 0x657093E: ffi_call \
(in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)<br> ==5331== by 0x4DDB183: ??? \
(in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)<br> ==5331== by \
0x4DD79D8: ??? (in /usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)<br> ==5331== \
by 0x4DD8EA3: wl_display_dispatch_queue_pending (in \
/usr/lib/x86_64-linux-gnu/libwayland-client.so.0.3.0)<br> ==5331== by 0x14BCCA: \
xwl_read_events (xwayland.c:814)<br> ==5331== by 0x2AC0D0: ospoll_wait \
(ospoll.c:651)<br> ==5331== by 0x2A5322: WaitForSomething (WaitFor.c:208)<br>
==5331== Block was alloc'd at<br>
==5331== at 0x48377D5: calloc (vg_replace_malloc.c:711)<br>
==5331== by 0x212D9F: present_vblank_create (present_vblank.c:69)<br>
==5331== by 0x214014: present_wnmd_pixmap (present_wnmd.c:610)<br>
==5331== by 0x21576C: proc_present_pixmap (present_request.c:150)<br>
==5331== by 0x27599D: Dispatch (dispatch.c:479)<br>
==5331== by 0x279945: dix_main (main.c:276)<br>
==5331== by 0x633AB16: (below main) (libc-start.c:310)<br>
<br>
Signed-off-by: Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com" \
target="_blank">lionel.g.landwerlin@intel.com</a>><br>
Reviewed-by: Daniel Stone <<a href="mailto:daniels@collabora.com" \
target="_blank">daniels@collabora.com</a>><br>
Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=107314" \
rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=107314</a><br>
---<br>
present/present_wnmd.c | 3 ++-<br>
1 file changed, 2 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/present/present_wnmd.c b/present/present_wnmd.c<br>
index 80ffb014e..48f66b357 100644<br>
--- a/present/present_wnmd.c<br>
+++ b/present/present_wnmd.c<br>
@@ -190,8 +190,9 @@ present_wnmd_flip_notify(present_vblank_ptr vblank, uint64_t ust, \
uint64_t crtc_<br> <br>
if (vblank->abort_flip)<br>
present_wnmd_flips_stop(window);<br>
+ else<br>
+ present_vblank_notify(vblank, PresentCompleteKindPixmap, \
PresentCompleteModeFlip, ust, crtc_msc);<br> <br>
- present_vblank_notify(vblank, PresentCompleteKindPixmap, \
PresentCompleteModeFlip, ust, crtc_msc);<br> \
present_wnmd_flip_try_ready(window);<br> }<br>
<br>
-- <br>
2.18.0<br>
<br>
_______________________________________________<br>
<a href="mailto:xorg-devel@lists.x.org" target="_blank">xorg-devel@lists.x.org</a>: \
X.Org development<br>
Archives: <a href="http://lists.x.org/archives/xorg-devel" rel="noreferrer" \
target="_blank">http://lists.x.org/archives/xorg-devel</a><br>
Info: <a href="https://lists.x.org/mailman/listinfo/xorg-devel" rel="noreferrer" \
target="_blank">https://lists.x.org/mailman/listinfo/xorg-devel</a></blockquote></div>
[Attachment #6 (text/plain)]
_______________________________________________
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic