[PATCH 4/8] Make xf86ValidateModes actually copy clock range list to screen pointer
Alan Coopersmith
alan.coopersmith at oracle.com
Fri Feb 1 23:32:07 PST 2013
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
More information about the xorg-devel
mailing list