[PATCH] privates: Clear screen-specific keys during CloseScreen

[Aaron Plattner]

On 09/16/2015 07:34 PM, Keith Packard wrote:
> Aaron Plattner <aplattner at nvidia.com> writes:
> 
>> The modesetting driver corrupts memory when used after a server regeneration
>> because not enough memory is allocated for its pixmap privates.  This happens
>> because its call to dixRegisterScreenSpecificPrivateKey() does nothing because
>> key->initialized is still TRUE from the first server generation.  However, the
>> key is not in the screen's linked list of screen-specific privates because
>> that's freed and reallocated during the server generation loop in dix_main().
>>
>> Fix this by clearing the screen-specific keys during CloseScreen, the same way
>> other privates are cleared during dixResetPrivates().
> 
> Yeah, this makes sense. I think that we expected the screen-specific
> privates to be allocated as a part of the screen initialization or
> privates (and hence why dixFreeScreenSpecificPrivates is called before
> CloseScreen when it might get freed if it were separately allocated).
> 
> I have a slight fear that CloseScreen is going to reference these
> privates, and so cleaning them before CloseScreen might turn out
> badly. A lot of code gets executed in CloseScreen, after all.

Yeah, that's fair.  I think it should be safe to move these calls to
after CloseScreen, but I'll have to think through it harder to convince
myself.

> There's no interface for screen-specific privates to be allocated by the
> privates.c code, so we can safely assume that they do not need to be
> freed themselves.

Unless the caller messed with the key internals, key->allocated should
be set to FALSE by dixRegisterScreenSpecificPrivateKey().

> How about we just set key->initialized = FALSE and leave the rest of the
> key alone? That does assume that no screens will get hot-added during
> CloseScreen, which seems entirely reasonable to me. The whole thing will
> then be neatly re-set when the server comes around again.

We could certainly just set key->initialized to FALSE instead, yeah. But
then why does dixResetPrivates() bother setting anything else?

I agree that adding new screens during the CloseScreen loop is asking
for trouble.  I hope that can't happen today...

> Alternatively, we could assert that dixRegisterScreenSpecificPrivateKey
> is only called once per key and remove the check for key->initialized,
> which is really only valid for keys used across multiple objects. I think?

I don't think that works because we need these keys to be in
pScreen->screenSpecificPrivates[*].key, and that's lost when the screen
is recycled.

-- 
Aaron