On Fri, 14 Feb 2003, David Dawes wrote:
[snip]
> There are some more serious things holding up 4.3, including the issue
> that Leif mentioned here a couple of days ago. I haven't seen anyone
> comment on his proposed solution for that one either...
>
> David
I was wondering about that myself. :) I'll reiterate the problem, but this
time with a patch to give people something more concrete to comment on.
This is, I think, the easiest and quickest fix, if not necessarily the
most correct. This patch does the following:
1. Move the __driGarbageCollectDrawables() call in driDestroyContext()
back after the driver's DestroyContext() callback as in the original patch
in the DRI mesa-4-0-4-branch. This avoids the issue of the drawable being
destroyed before the driver tries to reference it, at least in the
observed case in the Radeon driver's DestroyContext().
2. Free pBackClipRects in driDestroyDrawable(). This fixes a potential
memory leak if there are back-buffer cliprects when the drawable is
destroyed. I'm pretty sure this a seperate issue and a valid fix.
3. As an added sanity measure, set pClipRects and pBackClipRects pointers
to NULL in the drawable private struct when they are freed, before freeing
the struct. This _might_ prevent a mirrored private drawable struct
pointer held by a driver from pointing to invalid cliprect pointers.
However, it won't change the fact that the mirrored private drawable
struct pointer itself would be invalid. This change is pretty dubious, as
it has the potential to hide the actual problem. I don't think this
should take the place of fix #1. If the mirrored private drawable pointer
itself is invalid, dereferencing it produces undefined behavior.
If we don't use Fix #1, the alternative is to modify the drivers to set
the mirrored drawable pointer to NULL in the DestroyBuffer() callback and
deal with the possibility of a NULL drawable pointer in any code that can
be called after DestroyBuffer(), e.g when locking and any state changes
made after the lock is acquired. This might be a more correct fix, but it
seems to me it would be more intrusive and require more checking for
side-effects.
--
Leif Delgass
http://www.retinalburn.net
Index: dri_util.c
===================================================================
RCS file: /cvs/xc/lib/GL/dri/dri_util.c,v
retrieving revision 1.5
diff -u -r1.5 dri_util.c
--- dri_util.c 2003/02/11 03:30:20 1.5
+++ dri_util.c 2003/02/14 22:50:11
@@ -629,7 +629,7 @@
pdp->numClipRects = 0;
pdp->pClipRects = NULL;
pdp->numBackClipRects = 0;
- pdp->pBackClipRects = 0;
+ pdp->pBackClipRects = NULL;
}
else
pdp->pStamp = &(psp->pSAREA->drawableTable[pdp->index].stamp);
@@ -740,8 +740,14 @@
(*psp->DriverAPI.DestroyBuffer)(pdp);
if (__driWindowExists(dpy, pdp->draw))
(void)XF86DRIDestroyDrawable(dpy, scrn, pdp->draw);
- if (pdp->pClipRects)
+ if (pdp->pClipRects) {
Xfree(pdp->pClipRects);
+ pdp->pClipRects = NULL;
+ }
+ if (pdp->pBackClipRects) {
+ Xfree(pdp->pBackClipRects);
+ pdp->pBackClipRects = NULL;
+ }
Xfree(pdp);
}
}
@@ -763,8 +769,8 @@
psp->fullscreen = NULL;
}
}
- __driGarbageCollectDrawables(pcp->driScreenPriv->drawHash);
(*pcp->driScreenPriv->DriverAPI.DestroyContext)(pcp);
+ __driGarbageCollectDrawables(pcp->driScreenPriv->drawHash);
(void)XF86DRIDestroyContext(dpy, scrn, pcp->contextID);
Xfree(pcp);
}
