[PATCH 4/8] Make xf86ValidateModes actually copy clock range list to screen pointer

[Peter Hutterer]

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 <alan.coopersmith at oracle.com>
> >> ---
> >>  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 <peter.hutterer at who-t.net> 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