From f4a9332ad149ed15353a9c482563bdd042d0b403 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 27 Jan 2013 10:06:42 -0800 Subject: [PATCH 1/8] Handle failure to create counter in init_system_idle_counter Check for NULL pointer (which can be returned for multiple reasons) before trying to dereference it to add privates. To avoid memory leak in error path, delay malloc of privates until we're ready to add them. In case we do return NULL up through SyncInitDeviceIdleTime, handle the possibility of getting NULL passed back down to SyncRemoveDeviceIdleTime. As reported by parfait 1.1: Error: Null pointer dereference (CWE 476) Read from null pointer 'idle_time_counter' at line 2764 of xserver/Xext/sync.c in function 'init_system_idle_counter'. Function 'SyncCreateSystemCounter' may return constant 'NULL' at line 952, called at line 2756. Null pointer introduced at line 952 in function 'SyncCreateSystemCounter'. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- Xext/sync.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Xext/sync.c b/Xext/sync.c index 4d11992bb..9ae5b3981 100644 --- a/Xext/sync.c +++ b/Xext/sync.c @@ -2747,7 +2747,6 @@ init_system_idle_counter(const char *name, int deviceid) { CARD64 resolution; XSyncValue idle; - IdleCounterPriv *priv = malloc(sizeof(IdleCounterPriv)); SyncCounter *idle_time_counter; IdleTimeQueryValue(NULL, &idle); @@ -2758,10 +2757,14 @@ init_system_idle_counter(const char *name, int deviceid) IdleTimeQueryValue, IdleTimeBracketValues); - priv->deviceid = deviceid; - priv->value_less = priv->value_greater = NULL; + if (idle_time_counter != NULL) { + IdleCounterPriv *priv = malloc(sizeof(IdleCounterPriv)); - idle_time_counter->pSysCounterInfo->private = priv; + priv->value_less = priv->value_greater = NULL; + priv->deviceid = deviceid; + + idle_time_counter->pSysCounterInfo->private = priv; + } return idle_time_counter; } @@ -2786,6 +2789,6 @@ void SyncRemoveDeviceIdleTime(SyncCounter *counter) /* FreeAllResources() frees all system counters before the devices are shut down, check if there are any left before freeing the device's counter */ - if (!xorg_list_is_empty(&SysCounterList)) + if (counter && !xorg_list_is_empty(&SysCounterList)) xorg_list_del(&counter->pSysCounterInfo->entry); } From 48b94651205b175760904e448f94111d1ab85e13 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 27 Jan 2013 11:20:45 -0800 Subject: [PATCH 2/8] Stop leaking overlayWin in PanoramiXCompositeGetOverlayWindow error paths Found by parfait 1.1 code analyzer: Error: Memory leak (CWE 401) Memory leak of pointer 'overlayWin' allocated with malloc(72) at line 806 of composite/compext.c in function 'PanoramiXCompositeGetOverlayWindow'. pointer allocated at line 794 with malloc(72). leaks when rc != 0 at line 804. at line 816 of composite/compext.c in function 'PanoramiXCompositeGetOverlayWindow'. pointer allocated at line 794 with malloc(72). leaks when pOc == NULL at line 815. at line 825 of composite/compext.c in function 'PanoramiXCompositeGetOverlayWindow'. pointer allocated at line 794 with malloc(72). leaks when cs->pOverlayWin == NULL at line 822 and compCreateOverlayWindow(pScreen) == 0 at line 823. at line 834 of composite/compext.c in function 'PanoramiXCompositeGetOverlayWindow'. pointer allocated at line 794 with malloc(72). leaks when rc != 0 at line 832. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- composite/compext.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/composite/compext.c b/composite/compext.c index 8641eff5e..e4821c5fc 100644 --- a/composite/compext.c +++ b/composite/compext.c @@ -803,6 +803,7 @@ PanoramiXCompositeGetOverlayWindow(ClientPtr client) RT_WINDOW, client, DixGetAttrAccess); if (rc != Success) { client->errorValue = stuff->window; + free(overlayWin); return rc; } pScreen = pWin->drawable.pScreen; @@ -812,8 +813,10 @@ PanoramiXCompositeGetOverlayWindow(ClientPtr client) * interest in the overlay window */ pOc = compCreateOverlayClient(pScreen, client); - if (pOc == NULL) + if (pOc == NULL) { + free(overlayWin); return BadAlloc; + } /* * Make sure the overlay window exists @@ -822,6 +825,7 @@ PanoramiXCompositeGetOverlayWindow(ClientPtr client) if (cs->pOverlayWin == NULL) if (!compCreateOverlayWindow(pScreen)) { FreeResource(pOc->resource, RT_NONE); + free(overlayWin); return BadAlloc; } @@ -831,6 +835,7 @@ PanoramiXCompositeGetOverlayWindow(ClientPtr client) DixGetAttrAccess); if (rc != Success) { FreeResource(pOc->resource, RT_NONE); + free(overlayWin); return rc; } } From 89badba082c81d20fe35cb064c16e131ff288ca3 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 27 Jan 2013 12:08:47 -0800 Subject: [PATCH 3/8] Free keymap on error in Xephyr's hostx_load_keymap Found by parfait 1.1 code analyser: Memory leak of pointer 'keymap' allocated with XGetKeyboardMapping(HostX.dpy, min_keycode, ((max_keycode - min_keycode) + 1), &host_width) at line 861 of hw/kdrive/ephyr/hostx.c in function 'hostx_load_keymap'. 'keymap' allocated at line 845 with XGetKeyboardMapping(HostX.dpy, min_keycode, ((max_keycode - min_keycode) + 1), &host_width). Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- hw/kdrive/ephyr/hostx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/kdrive/ephyr/hostx.c b/hw/kdrive/ephyr/hostx.c index 157ac36b2..aed0285b6 100644 --- a/hw/kdrive/ephyr/hostx.c +++ b/hw/kdrive/ephyr/hostx.c @@ -858,7 +858,7 @@ hostx_load_keymap(void) (max_keycode - min_keycode + 1) * width); if (!ephyrKeySyms.map) - return; + goto out; for (i = 0; i < (max_keycode - min_keycode + 1); i++) for (j = 0; j < width; j++) @@ -871,6 +871,7 @@ hostx_load_keymap(void) ephyrKeySyms.maxKeyCode = max_keycode; ephyrKeySyms.mapWidth = width; + out: XFree(keymap); } From c1c01e350834a23161b33bd34b2fa9c01d02a65b Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 27 Jan 2013 13:10:08 -0800 Subject: [PATCH 4/8] Make xf86ValidateModes actually copy clock range list to screen pointer 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 then just for the fun of it, also sets storeClockRanges to the ->next pointer it has just copied from cp as well, even though it's going to overwrite it as the very first instruction in the loop body. v2: rewritten using nt_list_* macros from Xorg's list.h header Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- hw/xfree86/common/xf86Mode.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/hw/xfree86/common/xf86Mode.c b/hw/xfree86/common/xf86Mode.c index d80dec892..706ab64fc 100644 --- a/hw/xfree86/common/xf86Mode.c +++ b/hw/xfree86/common/xf86Mode.c @@ -1370,7 +1370,6 @@ xf86ValidateModes(ScrnInfoPtr scrp, DisplayModePtr availModes, int saveType; PixmapFormatRec *BankFormat; ClockRangePtr cp; - ClockRangePtr storeClockRanges; int numTimings = 0; range hsync[MAX_HSYNC]; range vrefresh[MAX_VREFRESH]; @@ -1492,16 +1491,14 @@ xf86ValidateModes(ScrnInfoPtr scrp, DisplayModePtr availModes, /* * Store the clockRanges for later use by the VidMode extension. */ - storeClockRanges = scrp->clockRanges; - while (storeClockRanges != NULL) { - storeClockRanges = storeClockRanges->next; - } - for (cp = clockRanges; cp != NULL; cp = cp->next, - storeClockRanges = storeClockRanges->next) { - storeClockRanges = xnfalloc(sizeof(ClockRange)); + 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 = storeClockRanges; - memcpy(storeClockRanges, cp, sizeof(ClockRange)); + scrp->clockRanges = newCR; + else + nt_list_append(newCR, scrp->clockRanges, ClockRange, next); } /* Determine which pixmap format to pass to scanLineWidth() */ From 08f75d3a9661c6c32800e1b4f150626200b889d9 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 27 Jan 2013 13:50:30 -0800 Subject: [PATCH 5/8] Avoid NULL pointer dereference in xf86TokenToOptinfo if token not found Reported by parfait 1.1 code analyzer: Error: Null pointer dereference (CWE 476) Read from null pointer 'p' at line 746 of hw/xfree86/common/xf86Option.c in function 'xf86TokenToOptName'. Function 'xf86TokenToOptinfo' may return constant 'NULL' at line 721, called at line 745. Null pointer introduced at line 721 in function 'xf86TokenToOptinfo'. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- hw/xfree86/common/xf86Option.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xfree86/common/xf86Option.c b/hw/xfree86/common/xf86Option.c index 40c9d15f4..607c33354 100644 --- a/hw/xfree86/common/xf86Option.c +++ b/hw/xfree86/common/xf86Option.c @@ -743,7 +743,7 @@ xf86TokenToOptName(const OptionInfoRec * table, int token) const OptionInfoRec *p; p = xf86TokenToOptinfo(table, token); - return p->name; + return p ? p->name : NULL; } Bool From 563db909bcf965b6103c1807bf9f00ede957077d Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 27 Jan 2013 13:55:50 -0800 Subject: [PATCH 6/8] Avoid memory leak on realloc failure in localRegisterFreeBoxCallback Also avoids leaving invalid pointers in structures if realloc had to move them elsewhere to make them larger. Found by parfait 1.1 code analyzer: Memory leak of pointer 'newCallbacks' allocated with realloc(((char*)offman->FreeBoxesUpdateCallback), (8 * (offman->NumCallbacks + 1))) at line 328 of hw/xfree86/common/xf86fbman.c in function 'localRegisterFreeBoxCallback'. 'newCallbacks' allocated at line 320 with realloc(((char*)offman->FreeBoxesUpdateCallback), (8 * (offman->NumCallbacks + 1))). newCallbacks leaks when newCallbacks != NULL at line 327. Memory leak of pointer 'newPrivates' allocated with realloc(((char*)offman->devPrivates), (8 * (offman->NumCallbacks + 1))) at line 328 of hw/xfree86/common/xf86fbman.c in function 'localRegisterFreeBoxCallback'. 'newPrivates' allocated at line 324 with realloc(((char*)offman->devPrivates), (8 * (offman->NumCallbacks + 1))). newPrivates leaks when newCallbacks == NULL at line 327. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- hw/xfree86/common/xf86fbman.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/hw/xfree86/common/xf86fbman.c b/hw/xfree86/common/xf86fbman.c index c2e7bab9f..4da6af2b6 100644 --- a/hw/xfree86/common/xf86fbman.c +++ b/hw/xfree86/common/xf86fbman.c @@ -320,15 +320,17 @@ localRegisterFreeBoxCallback(ScreenPtr pScreen, newCallbacks = realloc(offman->FreeBoxesUpdateCallback, sizeof(FreeBoxCallbackProcPtr) * (offman->NumCallbacks + 1)); + if (!newCallbacks) + return FALSE; + else + offman->FreeBoxesUpdateCallback = newCallbacks; newPrivates = realloc(offman->devPrivates, sizeof(DevUnion) * (offman->NumCallbacks + 1)); - - if (!newCallbacks || !newPrivates) + if (!newPrivates) return FALSE; - - offman->FreeBoxesUpdateCallback = newCallbacks; - offman->devPrivates = newPrivates; + else + offman->devPrivates = newPrivates; offman->FreeBoxesUpdateCallback[offman->NumCallbacks] = FreeBoxCallback; offman->devPrivates[offman->NumCallbacks].ptr = devPriv; From b1129a1f1771c9d1653cc15aae94a614f081638a Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 27 Jan 2013 14:00:54 -0800 Subject: [PATCH 7/8] xf86XvMCScreenInit: Avoid leak if dixRegisterPrivateKey fails Found by parfait 1.1 memory analyser: Memory leak of pointer 'pAdapt' allocated with malloc((88 * num_adaptors)) at line 162 of hw/xfree86/common/xf86xvmc.c in function 'xf86XvMCScreenInit'. 'pAdapt' allocated at line 158 with malloc((88 * num_adaptors)). Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- hw/xfree86/common/xf86xvmc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/xfree86/common/xf86xvmc.c b/hw/xfree86/common/xf86xvmc.c index 78a32bfe1..3169c054c 100644 --- a/hw/xfree86/common/xf86xvmc.c +++ b/hw/xfree86/common/xf86xvmc.c @@ -158,8 +158,10 @@ xf86XvMCScreenInit(ScreenPtr pScreen, if (!(pAdapt = malloc(sizeof(XvMCAdaptorRec) * num_adaptors))) return FALSE; - if (!dixRegisterPrivateKey(&XF86XvMCScreenKeyRec, PRIVATE_SCREEN, 0)) + if (!dixRegisterPrivateKey(&XF86XvMCScreenKeyRec, PRIVATE_SCREEN, 0)) { + free(pAdapt); return FALSE; + } if (!(pScreenPriv = malloc(sizeof(xf86XvMCScreenRec)))) { free(pAdapt); From 73974dd7ea9ca4d4cdd5464cb813088a6ee9770b Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sun, 27 Jan 2013 15:42:02 -0800 Subject: [PATCH 8/8] Avoid memory leak in ddc resort() if find_header() fails Call find_header first, returning on failure before calling malloc. Signed-off-by: Alan Coopersmith Reviewed-by: Peter Hutterer --- hw/xfree86/ddc/ddc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/ddc/ddc.c b/hw/xfree86/ddc/ddc.c index 28c969646..44c1d535c 100644 --- a/hw/xfree86/ddc/ddc.c +++ b/hw/xfree86/ddc/ddc.c @@ -91,15 +91,16 @@ resort(unsigned char *s_block) unsigned char *d_new, *d_ptr, *d_end, *s_ptr, *s_end; unsigned char tmp; + s_ptr = find_header(s_block); + if (!s_ptr) + return NULL; s_end = s_block + EDID1_LEN; + d_new = malloc(EDID1_LEN); if (!d_new) return NULL; d_end = d_new + EDID1_LEN; - s_ptr = find_header(s_block); - if (!s_ptr) - return NULL; for (d_ptr = d_new; d_ptr < d_end; d_ptr++) { tmp = *(s_ptr++); *d_ptr = tmp;