FreeResource() keeps clientTable[cid].elements up to date with the number of resources allocated to the client. The other free resource functions (FreeResourceByType(), FreeClientNeverRetainResources() and FreeClientResources()) don't maintain this invariant.
Typically, the only consequence is that the element count is too high and we end up allocating the hash table bigger than necessary. However, FreeResource() also relies on the element count to restart the search if the list of resources has been changed during a resource destruction callback. Since FreeResourceByType() doesn't update the count, if we call that from a resource destruction callback from FreeResource(), the loop isn't restarted and we end up following an invalid next pointer. Furthermore, LookupClientResourceComplex() and FreeClientNeverRetainResources() don't use the element count to detect if a callback deleted a resource and may end up following an invalid next pointer if the resource system is called into recursively. Signed-off-by: Kristian Høgsberg <[email protected]> --- dix/resource.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-) 2010/5/8 Keith Packard <[email protected]>: > On Fri, 7 May 2010 15:43:23 -0400, Kristian H=C3=B8gsberg <k...@bitplanet.= net> wrote: > >> Eeek, you're right. =C2=A0And this is different from the other cases >> because here we don't actually delete the resource which caused >> another resource to be added so we'll end up doing it again. =C2=A0I don= 't >> have a good answer... we can leave it non-reentrant (didn't seem to >> cause problems before, even when DRI1 apparently deleted resources >> from the callback), or we can decide that the "add resource in >> callback" case is not allowed. > > How about 'changing the resource database before returning FALSE is not > allowed'? Otherwise, this function just returns and doesn't look at the > DB again. All current users (dri, xfixes and xinerama) respect this > requirement. That makes sense. > So, I think we should just leave this function alone. I can certainly do that. Something like this look ok? Kristian diff --git a/dix/resource.c b/dix/resource.c index 91d0cfb..ab3762e 100644 --- a/dix/resource.c +++ b/dix/resource.c @@ -589,6 +589,7 @@ FreeResourceByType(XID id, RESTYPE type, Bool skipFree) res->value, TypeNameString(res->type)); #endif *prev = res->next; + clientTable[cid].elements--; CallResourceStateCallback(ResourceStateFreeing, res); @@ -734,12 +735,14 @@ FreeClientNeverRetainResources(ClientPtr client) ResourcePtr *resources; ResourcePtr this; ResourcePtr *prev; - int j; + int j, elements; + int *eltptr; if (!client) return; resources = clientTable[client->index].resources; + eltptr = &clientTable[client->index].elements; for (j=0; j < clientTable[client->index].buckets; j++) { prev = &resources[j]; @@ -753,11 +756,15 @@ FreeClientNeverRetainResources(ClientPtr client) this->value, TypeNameString(this->type)); #endif *prev = this->next; + clientTable[client->index].elements--; CallResourceStateCallback(ResourceStateFreeing, this); + elements = *eltptr; (*DeleteFuncs[rtype & TypeMask])(this->value, this->id); xfree(this); + if (*eltptr != elements) + prev = &resources[j]; /* prev may no longer be valid */ } else prev = &this->next; @@ -804,6 +811,7 @@ FreeClientResources(ClientPtr client) this->value, TypeNameString(this->type)); #endif *head = this->next; + clientTable[client->index].elements--; CallResourceStateCallback(ResourceStateFreeing, this); -- 1.7.0.1 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
