On Fri, Feb 01, 2013 at 11:32:07PM -0800, Alan Coopersmith wrote: > On 01/28/13 06:59 PM, Peter Hutterer wrote: > > 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. > > I'd forgotten about the nt_list interfaces, just remembered we couldn't switch > to xorg_list without breaking ABI at this point, since the list is part of the > ScrnInfoPtr. It's not the greatest fit, since the list head is just a > pointer > to the first element, not an actual element, but it does shorten the code down > a bit (though the expanded implementation is a tiny bit less efficient if > there > get to be a bunch of entries in this list due to the search for the list tail > each time - anyone who cares about that can rewrite to a doubly-linked > xorg_list > for the next ABI breaking merge window). > > These interfaces are still a bit new to me - does this look correct? > > /* > * Store the clockRanges for later use by the VidMode extension. > */ > nt_list_for_each_entry(cp, clockRanges, next) { > ClockRangePtr newCR = xnfalloc(sizeof(ClockRange)); > memcpy(newCR, cp, sizeof(ClockRange)); > newCR->next = NULL; > if (scrp->clockRanges == NULL) > scrp->clockRanges = newCR; > else > nt_list_append(newCR, scrp->clockRanges, ClockRange, next); > } > > (Completely deleting the clockRangeTail from the previous version of this > patch.)
yes, looks correct, thanks. Cheers, Peter _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
