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
