[PATCH xserver 4/5] randr: properly cleanup on crtc and output destroy

[Giuseppe Bilotta]

On Mon, Nov 6, 2017 at 10:54 PM, Adam Jackson <ajax at nwnk.net> wrote:
> On Sat, 2017-11-04 at 23:06 +0100, Giuseppe Bilotta wrote:
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta at gmail.com>
>> ---
>>  randr/rrcrtc.c   | 6 ++++++
>>  randr/rroutput.c | 5 +++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
>> index 2eb9fbdc8..90922484f 100644
>> --- a/randr/rrcrtc.c
>> +++ b/randr/rrcrtc.c
>> @@ -873,6 +873,11 @@ RRCrtcDestroyResource(void *value, XID pid)
>>              }
>>          }
>>
>> +        if (pScrPriv->numCrtcs == 0) {
>> +            free(pScrPriv->crtcs);
>> +            pScrPriv->crtcs = NULL;
>> +        }
>> +
>>          RRResourcesChanged(pScreen);
>>      }
>>
>
> Nack. Cleaning up the screen private belongs in CloseScreen or similar,
> it shouldn't be triggered from a resource destructor.

While I don't disagree with the principle of the objection, I'd like
to point out that not cleaning up in the resource destructor is a leak
source. All allocation points rely on num* (e.g. numCrtcs) to
determine whether to do a malloc or realloc, assumingly not trusting
the pointer to be properly NULL-initialized. This means that a
numCrtcs = 0 with a non-freed crtcs will lead to leaks, always.

For example, RRCrtcCreate does a check for numCrtcs and uses a realloc
if it's non-zero, and a malloc if it's zero. In a situation where we
do RRCrtcCreate followed by RRCrtcDestroy followed by another
RRCrtcCreate, we have a leak. While for the CRTCs the example may be
ficticious, this kind of behavior is actually seen for things such as
outputs (whose dynamic creation and destruction is an actual feature
of some drivers).

Considering we already do a lot of memory management for the screen
private “outside” of screen functions (including allocations and
moves), I thought the resource would be the simplest and safest (as
in: guarantees no leaks) thing to do.

I'm guessing the better approach would be to rework the allocation
points, replacing the num* checks, going straight for reallocs
everywhere, and ensuring that the pointer field is correctly
NULL-initialized?

-- 
Giuseppe "Oblomov" Bilotta