On Mon, Jan 28, 2013 at 05:08:38PM -0800, Alan Coopersmith wrote: > Our in-house parfait 1.1 code analysis tool complained that every exit > path from xf86ValidateModes() in hw/xfree86/common/xf86Mode.c leaks the > storeClockRanges allocation made at line 1501 with XNFalloc. > > Investigating, it seems that this code to copy the clock range list to > the clockRanges list in the screen pointer is just plain insane, and > according to git, has been since we first imported it from XFree86. > > We start at line 1495 by walking the linked list from scrp->clockRanges > until we find the end. But that was just a diversion, since we've found > the end and immediately forgotten it, and thus at 1499 we know that > storeClockRanges is NULL, but that's not a problem since we're going to > immediately overwrite that value as the first thing in the loop. > > So we move on through this loop at 1499, which takes us through the > linked list from the clockRanges variable, and for every entry in > that list allocates a new structure and copies cp to it. If we've > not filled in the screen's clockRanges pointer yet, we set it to > the first storeClockRanges we copied from cp. Otherwise, as best > I can tell, we just drop it into memory and let it leak away, as > parfait warned. > > And then we hit the loop action, which if we haven't hit the end of > the cp list, advances cp to the next item in the list, and for reasons > I can't begin to fathom sets storeClockRanges to the ->next pointer it > had just copied from cp as well, even though it's going to overwrite > it as the very first instruction in the loop. > > Signed-off-by: Alan Coopersmith <[email protected]> > --- > hw/xfree86/common/xf86Mode.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/hw/xfree86/common/xf86Mode.c b/hw/xfree86/common/xf86Mode.c > index d80dec8..8e51cac 100644 > --- a/hw/xfree86/common/xf86Mode.c > +++ b/hw/xfree86/common/xf86Mode.c > @@ -1370,7 +1370,7 @@ xf86ValidateModes(ScrnInfoPtr scrp, DisplayModePtr > availModes, > int saveType; > PixmapFormatRec *BankFormat; > ClockRangePtr cp; > - ClockRangePtr storeClockRanges; > + ClockRangePtr clockRangeTail; > int numTimings = 0; > range hsync[MAX_HSYNC]; > range vrefresh[MAX_VREFRESH]; > @@ -1492,16 +1492,22 @@ xf86ValidateModes(ScrnInfoPtr scrp, DisplayModePtr > availModes, > /* > * Store the clockRanges for later use by the VidMode extension. > */ > - storeClockRanges = scrp->clockRanges; > - while (storeClockRanges != NULL) { > - storeClockRanges = storeClockRanges->next; > + clockRangeTail = scrp->clockRanges; > + /* Find last entry in current list, so we can start appending there */ > + if (clockRangeTail != NULL) { > + while (clockRangeTail->next != NULL) { > + clockRangeTail = clockRangeTail->next; > + } > } > - for (cp = clockRanges; cp != NULL; cp = cp->next, > - storeClockRanges = storeClockRanges->next) { > - storeClockRanges = xnfalloc(sizeof(ClockRange)); > - if (scrp->clockRanges == NULL) > - scrp->clockRanges = storeClockRanges; > - memcpy(storeClockRanges, cp, sizeof(ClockRange)); > + for (cp = clockRanges; cp != NULL; cp = cp->next) { > + ClockRangePtr newCR = xnfalloc(sizeof(ClockRange)); > + memcpy(newCR, cp, sizeof(ClockRange)); > + newCR->next = NULL; > + if (clockRangeTail == NULL) > + scrp->clockRanges = newCR; > + else > + clockRangeTail->next = newCR; > + clockRangeTail = newCR; > } >
I'd use the nt_list interface for this, but either way, Reviewed-by: Peter Hutterer <[email protected]> for the series. Cheers, Peter _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
