On Mon, Nov 6, 2017 at 10:54 PM, Adam Jackson <a...@nwnk.net> wrote: > On Sat, 2017-11-04 at 23:06 +0100, Giuseppe Bilotta wrote: >> Signed-off-by: Giuseppe Bilotta <giuseppe.bilo...@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 _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel