From 42efd8db899ac916f6a452de1196be1e410a1d7e Mon Sep 17 00:00:00 2001 From: "Enrico Weigelt, metux IT consult" Date: Wed, 2 Oct 2024 17:23:10 +0200 Subject: [PATCH] (!1709) treewide: NULL-protect ScreenRec->DestroyPixmap() calls Right now, we're assuming that even when deep nesting involved, the proc vector is always set to a valid function. One the one hand it requires extra dummy procs in some cases, OTOH it's making upcoming refactoring of the code flow unnecessarily complex. The big plot (of subsequent commits) is splitting out the extension's (and possibly subsystem's) special logic out of the wrapping chain and let them be executed independently from the DDX/drivers - when applicable even only when the pixmap is really destroyed (not just unref'ed). (At some later point, it might even become be actually a valid situation that DestroyPixmap vector really being NULL.) See: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1754 See: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1755 Signed-off-by: Enrico Weigelt, metux IT consult --- Xext/shm.c | 5 +++-- Xext/xvmain.c | 5 +++-- exa/exa_classic.c | 11 +++++++---- exa/exa_driver.c | 8 +++++--- exa/exa_mixed.c | 5 +++-- glamor/glamor_egl.c | 5 +++-- miext/damage/damage.c | 3 ++- 7 files changed, 26 insertions(+), 16 deletions(-) diff --git a/Xext/shm.c b/Xext/shm.c index 091a95b50..c0e366b98 100644 --- a/Xext/shm.c +++ b/Xext/shm.c @@ -252,13 +252,14 @@ ShmDestroyPixmap(PixmapPtr pPixmap) ScreenPtr pScreen = pPixmap->drawable.pScreen; ShmScrPrivateRec *screen_priv = ShmGetScreenPriv(pScreen); void *shmdesc = NULL; - Bool ret; + Bool ret = TRUE; if (pPixmap->refcnt == 1) shmdesc = dixLookupPrivate(&pPixmap->devPrivates, shmPixmapPrivateKey); pScreen->DestroyPixmap = screen_priv->destroyPixmap; - ret = (*pScreen->DestroyPixmap) (pPixmap); + if (pScreen->DestroyPixmap) + ret = pScreen->DestroyPixmap(pPixmap); screen_priv->destroyPixmap = pScreen->DestroyPixmap; pScreen->DestroyPixmap = ShmDestroyPixmap; diff --git a/Xext/xvmain.c b/Xext/xvmain.c index 34b6f1df5..370db0a4a 100644 --- a/Xext/xvmain.c +++ b/Xext/xvmain.c @@ -373,13 +373,14 @@ static Bool XvDestroyPixmap(PixmapPtr pPix) { ScreenPtr pScreen = pPix->drawable.pScreen; - Bool status; + Bool status = TRUE; if (pPix->refcnt == 1) XvStopAdaptors(&pPix->drawable); SCREEN_PROLOGUE(pScreen, DestroyPixmap); - status = (*pScreen->DestroyPixmap) (pPix); + if (pScreen->DestroyPixmap) + status = pScreen->DestroyPixmap(pPix); SCREEN_EPILOGUE(pScreen, DestroyPixmap, XvDestroyPixmap); return status; diff --git a/exa/exa_classic.c b/exa/exa_classic.c index a9460f999..006b3862d 100644 --- a/exa/exa_classic.c +++ b/exa/exa_classic.c @@ -99,7 +99,8 @@ exaCreatePixmap_classic(ScreenPtr pScreen, int w, int h, int depth, if (pExaPixmap->fb_pitch > 131071) { swap(pExaScr, pScreen, DestroyPixmap); - pScreen->DestroyPixmap(pPixmap); + if (pScreen->DestroyPixmap) + pScreen->DestroyPixmap(pPixmap); swap(pExaScr, pScreen, DestroyPixmap); return NULL; } @@ -111,7 +112,8 @@ exaCreatePixmap_classic(ScreenPtr pScreen, int w, int h, int depth, if (pExaPixmap->pDamage == NULL) { swap(pExaScr, pScreen, DestroyPixmap); - pScreen->DestroyPixmap(pPixmap); + if (pScreen->DestroyPixmap) + pScreen->DestroyPixmap(pPixmap); swap(pExaScr, pScreen, DestroyPixmap); return NULL; } @@ -214,7 +216,7 @@ exaDestroyPixmap_classic(PixmapPtr pPixmap) ScreenPtr pScreen = pPixmap->drawable.pScreen; ExaScreenPriv(pScreen); - Bool ret; + Bool ret = TRUE; if (pPixmap->refcnt == 1) { ExaPixmapPriv(pPixmap); @@ -236,7 +238,8 @@ exaDestroyPixmap_classic(PixmapPtr pPixmap) } swap(pExaScr, pScreen, DestroyPixmap); - ret = pScreen->DestroyPixmap(pPixmap); + if (pScreen->DestroyPixmap) + ret = pScreen->DestroyPixmap(pPixmap); swap(pExaScr, pScreen, DestroyPixmap); return ret; diff --git a/exa/exa_driver.c b/exa/exa_driver.c index 7a74c4500..f7c2e5d28 100644 --- a/exa/exa_driver.c +++ b/exa/exa_driver.c @@ -101,7 +101,8 @@ exaCreatePixmap_driver(ScreenPtr pScreen, int w, int h, int depth, if (!pExaPixmap->driverPriv) { swap(pExaScr, pScreen, DestroyPixmap); - pScreen->DestroyPixmap(pPixmap); + if (pScreen->DestroyPixmap) + pScreen->DestroyPixmap(pPixmap); swap(pExaScr, pScreen, DestroyPixmap); return NULL; } @@ -193,7 +194,7 @@ exaDestroyPixmap_driver(PixmapPtr pPixmap) ScreenPtr pScreen = pPixmap->drawable.pScreen; ExaScreenPriv(pScreen); - Bool ret; + Bool ret = TRUE; if (pPixmap->refcnt == 1) { ExaPixmapPriv(pPixmap); @@ -206,7 +207,8 @@ exaDestroyPixmap_driver(PixmapPtr pPixmap) } swap(pExaScr, pScreen, DestroyPixmap); - ret = pScreen->DestroyPixmap(pPixmap); + if (pScreen->DestroyPixmap) + ret = pScreen->DestroyPixmap(pPixmap); swap(pExaScr, pScreen, DestroyPixmap); return ret; diff --git a/exa/exa_mixed.c b/exa/exa_mixed.c index 4c7714657..cda15e7f7 100644 --- a/exa/exa_mixed.c +++ b/exa/exa_mixed.c @@ -247,7 +247,7 @@ exaDestroyPixmap_mixed(PixmapPtr pPixmap) ScreenPtr pScreen = pPixmap->drawable.pScreen; ExaScreenPriv(pScreen); - Bool ret; + Bool ret = TRUE; if (pPixmap->refcnt == 1) { ExaPixmapPriv(pPixmap); @@ -269,7 +269,8 @@ exaDestroyPixmap_mixed(PixmapPtr pPixmap) } swap(pExaScr, pScreen, DestroyPixmap); - ret = pScreen->DestroyPixmap(pPixmap); + if (pScreen->DestroyPixmap) + ret = pScreen->DestroyPixmap(pPixmap); swap(pExaScr, pScreen, DestroyPixmap); return ret; diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c index 5e0cbb035..25abaf28f 100644 --- a/glamor/glamor_egl.c +++ b/glamor/glamor_egl.c @@ -757,7 +757,7 @@ glamor_egl_destroy_pixmap(PixmapPtr pixmap) ScrnInfoPtr scrn = xf86ScreenToScrn(screen); struct glamor_egl_screen_private *glamor_egl = glamor_egl_get_screen_private(scrn); - Bool ret; + Bool ret = TRUE; if (pixmap->refcnt == 1) { struct glamor_pixmap_private *pixmap_priv = @@ -768,7 +768,8 @@ glamor_egl_destroy_pixmap(PixmapPtr pixmap) } screen->DestroyPixmap = glamor_egl->saved_destroy_pixmap; - ret = screen->DestroyPixmap(pixmap); + if (screen->DestroyPixmap) + ret = screen->DestroyPixmap(pixmap); glamor_egl->saved_destroy_pixmap = screen->DestroyPixmap; screen->DestroyPixmap = glamor_egl_destroy_pixmap; diff --git a/miext/damage/damage.c b/miext/damage/damage.c index 0ed392ca2..c1f45c951 100644 --- a/miext/damage/damage.c +++ b/miext/damage/damage.c @@ -1504,7 +1504,8 @@ damageDestroyPixmap(PixmapPtr pPixmap) } } unwrap(pScrPriv, pScreen, DestroyPixmap); - (*pScreen->DestroyPixmap) (pPixmap); + if (pScreen->DestroyPixmap) + pScreen->DestroyPixmap(pPixmap); wrap(pScrPriv, pScreen, DestroyPixmap, damageDestroyPixmap); return TRUE; }