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

[Alan Coopersmith]

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.)

-- 
	-Alan Coopersmith-              alan.coopersmith at oracle.com
	 Oracle Solaris Engineering - http://blogs.oracle.com/alanc