Re: [Dri-devel] Re: Some DRM related changes for running multiplei810/830M X servers
Leif Delgass wrote: 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(). As per our earlier discussions about this patch, I think that this is absolutely correct. The old code is clearly wrong. Does anybody know why the patch was reverted??? 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. That seems okay too. Is there any way to verify that the memory leak could actually ever happen? 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. For debugging, would we just over-write the whole private drawable struct with garbage? Is there anyway to back track and NULL out the mirrored pointer when the private drawable is destroyed? --- This SF.NET email is sponsored by: FREE SSL Guide from Thawte are you planning your Web Server Security? Click here to get a FREE Thawte SSL guide and find the answers to all your SSL security issues. http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [Dri-devel] Re: Some DRM related changes for running multiplei810/830M X servers
On Fri, 14 Feb 2003, Ian Romanick wrote: Leif Delgass wrote: 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(). As per our earlier discussions about this patch, I think that this is absolutely correct. The old code is clearly wrong. Does anybody know why the patch was reverted??? As far as I can tell, an additional fix for the infinite loop on getting the drawable info was added (using __driFindDrawable()), and it was assumed that this part of the patch wasn't needed. Probably the double-free issue wasn't known to be there? (There's no segfault, so you'd have to use MALLOC_CHECK_ to see it in testing). 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. That seems okay too. Is there any way to verify that the memory leak could actually ever happen? I put a printf in the block where pBackClipRects is freed (in the patched version), and I can get it to trigger when closing multiple texobj instances, e.g. I ran them both with MALLOC_CHECK_, and there's no double-free happening. So yes, it seems it can happen. 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. For debugging, would we just over-write the whole private drawable struct with garbage? Is there anyway to back track and NULL out the mirrored pointer when the private drawable is destroyed? Well, as I mentioned, the driDestroyDrawable() function calls the driver's DestroyBuffer() right before freeing the private struct, so the driver could NULL out it's mirrored pointer then. But that means checking for a NULL private drawable in any code that could be reached after that point. __driUtilUpdateDrawableInfo() should never be called with a NULL or invalid private drawable pointer as it stands now, as the result of freeing the cliprects is undefined behavior (even with Fix #3). Moving the __driGarbageCollectDrawables() call avoids all this, at least for context teardown. The other place __driGarbageCollectDrawables() is called is at the end of driCreateContext(). At that point, the driver's CreateContext() has been called, which initializes the drawable private pointer to NULL. The pointer first gets a real value when the context is made current. I don't see any other way that the drawable could be destroyed before the driver's context is, with Fix #1 in place. -- Leif Delgass http://www.retinalburn.net --- This SF.NET email is sponsored by: FREE SSL Guide from Thawte are you planning your Web Server Security? Click here to get a FREE Thawte SSL guide and find the answers to all your SSL security issues. http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en ___ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [Dri-devel] Re: Some DRM related changes for running multiplei810/830M X servers
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);
}
