From 6a8ee31e1b3a14f3cba7e0d1e1959da9a1e35170 Mon Sep 17 00:00:00 2001 From: "Enrico Weigelt, metux IT consult" Date: Tue, 6 May 2025 13:26:38 +0200 Subject: [PATCH] dix: move props into WindowRec and fix potential NULL deref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pointer to the window properties is currently inside the WindowOptional structure, which may or may not exist at any given time. Thus, before accessing those fields, at least need to check whether it exists, potentially need to create it first. Since a pointer is small (in relation to WindowRec) and windows having properties is a pretty common, we can make our life much simpler here by moving the pointer directly into WindowRec, so we don't need extra WindowOptionalRec allocation. This also fixes an analyzer warning on potential NULL dereference issue: | ../dix/property.c: In function ‘dixChangeWindowProperty’: |../dix/property.c:343:37: warning: dereference of NULL ‘*pWin.optional’ [CWE-476] [-Wanalyzer-null-dereference] | 343 | pProp->next = pWin->optional->userProps; | | ~~~~~~~~~~~~~~^~~~~~~~~~~ Signed-off-by: Enrico Weigelt, metux IT consult --- Xext/xselinux_ext.c | 4 +-- dix/property.c | 32 +++++++++----------- dix/window.c | 6 +--- hw/xwin/winmultiwindowclass.c | 57 ++++------------------------------- include/windowstr.h | 6 ++-- 5 files changed, 28 insertions(+), 77 deletions(-) diff --git a/Xext/xselinux_ext.c b/Xext/xselinux_ext.c index faf401634..5fe7fdc86 100644 --- a/Xext/xselinux_ext.c +++ b/Xext/xselinux_ext.c @@ -423,7 +423,7 @@ ProcSELinuxListProperties(ClientPtr client) /* Count the number of properties and allocate items */ count = 0; - for (pProp = wUserProps(pWin); pProp; pProp = pProp->next) + for (pProp = pWin->properties; pProp; pProp = pProp->next) count++; items = calloc(count, sizeof(SELinuxListItemRec)); if (count && !items) @@ -432,7 +432,7 @@ ProcSELinuxListProperties(ClientPtr client) /* Fill in the items and calculate size */ i = 0; size = 0; - for (pProp = wUserProps(pWin); pProp; pProp = pProp->next) { + for (pProp = pWin->properties; pProp; pProp = pProp->next) { id = pProp->propertyName; rc = SELinuxPopulateItem(items + i, &pProp->devPrivates, id, &size); if (rc != Success) { diff --git a/dix/property.c b/dix/property.c index 5e2ddfcf5..032b51ea2 100644 --- a/dix/property.c +++ b/dix/property.c @@ -77,7 +77,7 @@ PrintPropertys(WindowPtr pWin) PropertyPtr pProp; int j; - pProp = pWin->userProps; + pProp = pWin->properties; while (pProp) { ErrorF("[dix] %x %x\n", pProp->propertyName, pProp->type); ErrorF("[dix] property format: %d\n", pProp->format); @@ -98,7 +98,7 @@ dixLookupProperty(PropertyPtr *result, WindowPtr pWin, Atom propertyName, client->errorValue = propertyName; - for (pProp = wUserProps(pWin); pProp; pProp = pProp->next) + for (pProp = pWin->properties; pProp; pProp = pProp->next) if (pProp->propertyName == propertyName) break; @@ -299,8 +299,8 @@ dixChangeWindowProperty(ClientPtr pClient, WindowPtr pWin, Atom property, pClient->errorValue = property; return rc; } - pProp->next = pWin->optional->userProps; - pWin->optional->userProps = pProp; + pProp->next = pWin->properties; + pWin->properties = pProp; } else if (rc == Success) { /* To append or prepend to a property the request format and type @@ -384,14 +384,14 @@ DeleteProperty(ClientPtr client, WindowPtr pWin, Atom propName) return Success; /* Succeed if property does not exist */ if (rc == Success) { - if (pWin->optional->userProps == pProp) { + if (pWin->properties == pProp) { /* Takes care of head */ - if (!(pWin->optional->userProps = pProp->next)) + if (!(pWin->properties = pProp->next)) CheckWindowOptionalNeed(pWin); } else { /* Need to traverse to find the previous element */ - prevProp = pWin->optional->userProps; + prevProp = pWin->properties; while (prevProp->next != pProp) prevProp = prevProp->next; prevProp->next = pProp->next; @@ -407,19 +407,17 @@ DeleteProperty(ClientPtr client, WindowPtr pWin, Atom propName) void DeleteAllWindowProperties(WindowPtr pWin) { - PropertyPtr pProp, pNextProp; + PropertyPtr pProp = pWin->properties; - pProp = wUserProps(pWin); while (pProp) { deliverPropertyNotifyEvent(pWin, PropertyDelete, pProp); - pNextProp = pProp->next; + PropertyPtr pNextProp = pProp->next; free(pProp->data); dixFreeObjectWithPrivates(pProp, PRIVATE_PROPERTY); pProp = pNextProp; } - if (pWin->optional) - pWin->optional->userProps = NULL; + pWin->properties = NULL; } static int @@ -554,14 +552,14 @@ ProcGetProperty(ClientPtr client) if (stuff->delete && (reply.bytesAfter == 0)) { /* Delete the Property */ - if (pWin->optional->userProps == pProp) { + if (pWin->properties == pProp) { /* Takes care of head */ - if (!(pWin->optional->userProps = pProp->next)) + if (!(pWin->properties = pProp->next)) CheckWindowOptionalNeed(pWin); } else { /* Need to traverse to find the previous element */ - prevProp = pWin->optional->userProps; + prevProp = pWin->properties; while (prevProp->next != pProp) prevProp = prevProp->next; prevProp->next = pProp->next; @@ -589,7 +587,7 @@ ProcListProperties(ClientPtr client) if (rc != Success) return rc; - for (pProp = wUserProps(pWin); pProp; pProp = pProp->next) + for (pProp = pWin->properties; pProp; pProp = pProp->next) numProps++; if (numProps) { @@ -599,7 +597,7 @@ ProcListProperties(ClientPtr client) numProps = 0; temppAtoms = pAtoms; - for (pProp = wUserProps(pWin); pProp; pProp = pProp->next) { + for (pProp = pWin->properties; pProp; pProp = pProp->next) { realProp = pProp; rc = XaceHookPropertyAccess(client, pWin, &realProp, DixGetAttrAccess); if (rc == Success && realProp == pProp) { diff --git a/dix/window.c b/dix/window.c index f870ad99e..3b34f8486 100644 --- a/dix/window.c +++ b/dix/window.c @@ -205,7 +205,7 @@ get_window_name(WindowPtr pWin) return overlay_win_name; #endif - for (prop = wUserProps(pWin); prop; prop = prop->next) { + for (prop = pWin->properties; prop; prop = prop->next) { if (prop->propertyName == XA_WM_NAME && prop->type == XA_STRING && prop->data) { len = min(prop->size, WINDOW_NAME_BUF_LEN - 1); @@ -599,7 +599,6 @@ CreateRootWindow(ScreenPtr pScreen) pWin->optional->otherEventMasks = 0; pWin->optional->otherClients = NULL; pWin->optional->passiveGrabs = NULL; - pWin->optional->userProps = NULL; pWin->optional->backingBitPlanes = ~0L; pWin->optional->backingPixel = 0; pWin->optional->boundingShape = NULL; @@ -3349,8 +3348,6 @@ CheckWindowOptionalNeed(WindowPtr w) return; if (optional->passiveGrabs != NULL) return; - if (optional->userProps != NULL) - return; if (optional->backingBitPlanes != (CARD32)~0L) return; if (optional->backingPixel != 0) @@ -3406,7 +3403,6 @@ MakeWindowOptional(WindowPtr pWin) optional->otherEventMasks = 0; optional->otherClients = NULL; optional->passiveGrabs = NULL; - optional->userProps = NULL; optional->backingBitPlanes = ~0L; optional->backingPixel = 0; optional->boundingShape = NULL; diff --git a/hw/xwin/winmultiwindowclass.c b/hw/xwin/winmultiwindowclass.c index 6787332a3..0d5ce3b1c 100644 --- a/hw/xwin/winmultiwindowclass.c +++ b/hw/xwin/winmultiwindowclass.c @@ -46,8 +46,6 @@ DEFINE_ATOM_HELPER(AtmWmWindowRole, "WM_WINDOW_ROLE") int winMultiWindowGetClassHint(WindowPtr pWin, char **res_name, char **res_class) { - struct _Window *pwin; - struct _Property *prop; int len_name, len_class; if (!pWin || !res_name || !res_class) { @@ -56,12 +54,7 @@ winMultiWindowGetClassHint(WindowPtr pWin, char **res_name, char **res_class) return 0; } - pwin = (struct _Window *) pWin; - - if (pwin->optional) - prop = (struct _Property *) pwin->optional->userProps; - else - prop = NULL; + PropertyPtr prop = pWin->properties; *res_name = *res_class = NULL; @@ -116,20 +109,12 @@ winMultiWindowGetClassHint(WindowPtr pWin, char **res_name, char **res_class) int winMultiWindowGetWMHints(WindowPtr pWin, WinXWMHints * hints) { - struct _Window *pwin; - struct _Property *prop; - if (!pWin || !hints) { ErrorF("winMultiWindowGetWMHints - pWin or hints was NULL\n"); return 0; } - pwin = (struct _Window *) pWin; - - if (pwin->optional) - prop = (struct _Property *) pwin->optional->userProps; - else - prop = NULL; + PropertyPtr prop = pWin->properties; memset(hints, 0, sizeof(WinXWMHints)); @@ -148,19 +133,12 @@ winMultiWindowGetWMHints(WindowPtr pWin, WinXWMHints * hints) int winMultiWindowGetWindowRole(WindowPtr pWin, char **res_role) { - struct _Window *pwin; - struct _Property *prop; int len_role; if (!pWin || !res_role) return 0; - pwin = (struct _Window *) pWin; - - if (pwin->optional) - prop = (struct _Property *) pwin->optional->userProps; - else - prop = NULL; + PropertyPtr prop = pWin->properties; *res_role = NULL; while (prop) { @@ -190,20 +168,12 @@ winMultiWindowGetWindowRole(WindowPtr pWin, char **res_role) int winMultiWindowGetWMNormalHints(WindowPtr pWin, WinXSizeHints * hints) { - struct _Window *pwin; - struct _Property *prop; - if (!pWin || !hints) { ErrorF("winMultiWindowGetWMNormalHints - pWin or hints was NULL\n"); return 0; } - pwin = (struct _Window *) pWin; - - if (pwin->optional) - prop = (struct _Property *) pwin->optional->userProps; - else - prop = NULL; + PropertyPtr prop = pWin->properties; memset(hints, 0, sizeof(WinXSizeHints)); @@ -222,20 +192,12 @@ winMultiWindowGetWMNormalHints(WindowPtr pWin, WinXSizeHints * hints) int winMultiWindowGetTransientFor(WindowPtr pWin, Window *pDaddyId) { - struct _Window *pwin; - struct _Property *prop; - if (!pWin) { ErrorF("winMultiWindowGetTransientFor - pWin was NULL\n"); return 0; } - pwin = (struct _Window *) pWin; - - if (pwin->optional) - prop = (struct _Property *) pwin->optional->userProps; - else - prop = NULL; + PropertyPtr prop = pWin->properties; if (pDaddyId) *pDaddyId = 0; @@ -256,8 +218,6 @@ winMultiWindowGetTransientFor(WindowPtr pWin, Window *pDaddyId) int winMultiWindowGetWMName(WindowPtr pWin, char **wmName) { - struct _Window *pwin; - struct _Property *prop; int len_name; if (!pWin || !wmName) { @@ -266,12 +226,7 @@ winMultiWindowGetWMName(WindowPtr pWin, char **wmName) return 0; } - pwin = (struct _Window *) pWin; - - if (pwin->optional) - prop = (struct _Property *) pwin->optional->userProps; - else - prop = NULL; + PropertyPtr prop = pWin->properties; *wmName = NULL; diff --git a/include/windowstr.h b/include/windowstr.h index c7e27965e..94d2615fb 100644 --- a/include/windowstr.h +++ b/include/windowstr.h @@ -85,7 +85,6 @@ typedef struct _WindowOpt { Mask otherEventMasks; /* default: 0 */ struct _OtherClients *otherClients; /* default: NULL */ struct _GrabRec *passiveGrabs; /* default: NULL */ - PropertyPtr userProps; /* default: NULL */ CARD32 backingBitPlanes; /* default: ~0L */ CARD32 backingPixel; /* default: 0 */ RegionPtr boundingShape; /* default: NULL */ @@ -165,6 +164,8 @@ typedef struct _Window { unsigned damagedDescendants:1; /* some descendants are damaged */ unsigned inhibitBGPaint:1; /* paint the background? */ #endif + + PropertyPtr properties; /* default: NULL */ } WindowRec; /* @@ -189,7 +190,6 @@ extern _X_EXPORT Mask DontPropagateMasks[]; #define wOtherClients(w) wUseDefault(w, otherClients, NULL) #define wOtherInputMasks(w) wUseDefault(w, inputMasks, NULL) #define wPassiveGrabs(w) wUseDefault(w, passiveGrabs, NULL) -#define wUserProps(w) wUseDefault(w, userProps, NULL) #define wBackingBitPlanes(w) wUseDefault(w, backingBitPlanes, ~0L) #define wBackingPixel(w) wUseDefault(w, backingPixel, 0) #define wBoundingShape(w) wUseDefault(w, boundingShape, NULL) @@ -198,6 +198,8 @@ extern _X_EXPORT Mask DontPropagateMasks[]; #define wClient(w) (clients[CLIENT_ID((w)->drawable.id)]) #define wBorderWidth(w) ((int) (w)->borderWidth) +static inline PropertyPtr wUserProps(WindowPtr pWin) { return pWin->properties; } + /* true when w needs a border drawn. */ #define HasBorder(w) ((w)->borderWidth || wClipShape(w))