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

[Peter Hutterer]

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.

Cheers,
   Peter