[PATCH 5/6] present: Do not free screen_priv->flip_pending until it is finished

[Chris Wilson]

On Wed, May 28, 2014 at 12:22:14PM +0100, Chris Wilson wrote:
> On Wed, May 28, 2014 at 12:07:40PM +0100, Frank Binns wrote:
> > 
> > On 28/05/14 08:14, Chris Wilson wrote:
> > >If we close the Window prior to completing all the flips, we free the
> > >vblank pointer but leave screen_priv->flip_pending dangling. This leads
> > >to a host of invalid reads/writes as it is accessed from many locations,
> > >e.g.:
> > >
> > >==20302== Invalid write of size 4
> > >==20302==    at 0x81A8FD5: present_flip_destroy (present.c:879)
> > >==20302==    by 0x81A6A2A: present_close_screen (present_screen.c:60)
> > >==20302==    by 0x8147A5A: CursorCloseScreen (cursor.c:187)
> > >==20302==    by 0x81A3DDA: AnimCurCloseScreen (animcur.c:106)
> > >==20302==    by 0x8142F7D: compCloseScreen (compinit.c:85)
> > >==20302==    by 0x80E6D2A: VidModeClose (xf86VidMode.c:111)
> > >==20302==    by 0x496CF23: glxCloseScreen (glxscreens.c:187)
> > >==20302==    by 0x808186F: dix_main (main.c:349)
> > >==20302==    by 0x80D2FA1: main (stubmain.c:34)
> > >==20302==  Address 0x67d0780 is 104 bytes inside a block of size 108 free'd
> > >==20302==    at 0x4007BCD: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
> > >==20302==    by 0x81A90AC: present_vblank_destroy (present.c:911)
> > >==20302==    by 0x81A6ADF: present_free_window_vblank (present_screen.c:79)
> > >==20302==    by 0x81A6C2C: present_destroy_window (present_screen.c:116)
> > >==20302==    by 0x8145796: compDestroyWindow (compwindow.c:608)
> > >==20302==    by 0x8176B6B: DbeDestroyWindow (dbe.c:1318)
> > >==20302==    by 0x80B5A98: FreeWindowResources (window.c:910)
> > >==20302==    by 0x80B5D7B: DeleteWindow (window.c:978)
> > >==20302==    by 0x80A9C85: doFreeResource (resource.c:873)
> > >==20302==    by 0x80AA5F8: FreeClientResources (resource.c:1139)
> > >==20302==    by 0x807BD5E: CloseDownClient (dispatch.c:3384)
> > >==20302==    by 0x80740B5: Dispatch (dispatch.c:406)
> > >
> > >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > >---
> > >  present/present_screen.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > >diff --git a/present/present_screen.c b/present/present_screen.c
> > >index b475d03..0a525a8 100644
> > >--- a/present/present_screen.c
> > >+++ b/present/present_screen.c
> > >@@ -72,11 +72,14 @@ static void
> > >  present_free_window_vblank(WindowPtr window)
> > >  {
> > >      present_window_priv_ptr     window_priv = present_window_priv(window);
> > >+    present_screen_priv_ptr     screen_priv = present_screen_priv(window->drawable.pScreen);
> > >+    present_vblank_ptr          flip_pending = screen_priv->flip_pending;
> > >      present_vblank_ptr          vblank, tmp;
> > >      xorg_list_for_each_entry_safe(vblank, tmp, &window_priv->vblank, window_list) {
> > >          present_abort_vblank(window->drawable.pScreen, vblank->crtc, vblank->event_id, vblank->target_msc);
> > >-        present_vblank_destroy(vblank);
> > >+        if (vblank != flip_pending)
> > >+            present_vblank_destroy(vblank);
> > >      }
> > >  }
> > 
> > Hi Chris,
> > Doesn't this result in the vblank structure never being cleaned up
> > because the vblank event, which would normally result in it being
> > destroyed, gets ignored due to present_abort_vblank() being called
> > a couple of lines above?
> 
> True, I was thinking it would still get cleaned up through
> present_flip_notify, which now never occurs.

Except that it somehow still does even after your 4 patches, leading to
a use-after-free of screen_priv->flip_pending.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre