From aa6f84021aaae145f54ebf98787e363e1c2022c6 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Sat, 20 Feb 2021 17:24:14 -0800 Subject: [PATCH] xquartz: Allocate each fbconfig separately A change during the 1.20 development cycle resulted in fbconfigs being walked and deallocated individually during __glXScreenDestroy. This change now avoids a use-after-free caused by that change. ==50859==ERROR: AddressSanitizer: heap-use-after-free on address 0x00010d3819c8 at pc 0x0001009d4230 bp 0x00016feca7a0 sp 0x00016feca798 READ of size 8 at 0x00010d3819c8 thread T5 #0 0x1009d422c in __glXScreenDestroy glxscreens.c:448 #1 0x10091cc98 in __glXAquaScreenDestroy indirect.c:510 #2 0x1009d2734 in glxCloseScreen glxscreens.c:169 #3 0x100740a24 in dix_main main.c:325 #4 0x10023ed50 in server_thread quartzStartup.c:65 #5 0x199ae7fd0 in _pthread_start+0x13c (libsystem_pthread.dylib:arm64e+0x6fd0) #6 0x199ae2d38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1d38) 0x00010d3819c8 is located 200 bytes inside of 12800-byte region [0x00010d381900,0x00010d384b00) freed by thread T5 here: #0 0x101477ba8 in wrap_free+0x98 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3fba8) #1 0x1009d4240 in __glXScreenDestroy glxscreens.c:449 #2 0x10091cc98 in __glXAquaScreenDestroy indirect.c:510 #3 0x1009d2734 in glxCloseScreen glxscreens.c:169 #4 0x100740a24 in dix_main main.c:325 #5 0x10023ed50 in server_thread quartzStartup.c:65 #6 0x199ae7fd0 in _pthread_start+0x13c (libsystem_pthread.dylib:arm64e+0x6fd0) #7 0x199ae2d38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1d38) previously allocated by thread T5 here: #0 0x101477e38 in wrap_calloc+0x9c (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3fe38) #1 0x100925a40 in __glXAquaCreateVisualConfigs visualConfigs.c:116 #2 0x10091cb24 in __glXAquaScreenProbe+0x224 (X11.bin:arm64+0x100730b24) #3 0x1009cd840 in xorgGlxServerInit glxext.c:528 #4 0x10074539c in _CallCallbacks dixutils.c:743 #5 0x100932a70 in CallCallbacks callback.h:83 #6 0x100932478 in GlxExtensionInit vndext.c:244 #7 0x10020a364 in InitExtensions miinitext.c:267 #8 0x10073fe7c in dix_main main.c:197 #9 0x10023ed50 in server_thread quartzStartup.c:65 #10 0x199ae7fd0 in _pthread_start+0x13c (libsystem_pthread.dylib:arm64e+0x6fd0) #11 0x199ae2d38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1d38) Regressed-in: 4b0a3cbab131eb453e2b3fc0337121969258a7be CC: Giuseppe Bilotta Signed-off-by: Jeremy Huddleston Sequoia (cherry picked from commit 487286d47260782d331229af10df17711cbca1ea) --- hw/xquartz/GL/visualConfigs.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/hw/xquartz/GL/visualConfigs.c b/hw/xquartz/GL/visualConfigs.c index 69b18f6a0..d810e2dfe 100644 --- a/hw/xquartz/GL/visualConfigs.c +++ b/hw/xquartz/GL/visualConfigs.c @@ -59,7 +59,7 @@ /* Based originally on code from indirect.c which was based on code from i830_dri.c. */ __GLXconfig *__glXAquaCreateVisualConfigs(int *numConfigsPtr, int screenNumber) { int numConfigs = 0; - __GLXconfig *visualConfigs, *c; + __GLXconfig *visualConfigs, *c, *l; struct glCapabilities caps; struct glCapabilitiesConfig *conf; int stereo, depth, aux, buffers, stencil, accum, color, msample; @@ -113,14 +113,13 @@ __GLXconfig *__glXAquaCreateVisualConfigs(int *numConfigsPtr, int screenNumber) if(numConfigsPtr) *numConfigsPtr = numConfigs; - visualConfigs = calloc(sizeof(*visualConfigs), numConfigs); - - if(NULL == visualConfigs) { - ErrorF("xcalloc failure when allocating visualConfigs\n"); - freeGlCapabilities(&caps); - return NULL; - } + /* Note that as of 1.20.0, we cannot allocate all the configs at once. + * __glXScreenDestroy now walks all the fbconfigs and frees them one at a time. + * See 4b0a3cbab131eb453e2b3fc0337121969258a7be. + */ + visualConfigs = calloc(sizeof(*visualConfigs), 1); + l = NULL; c = visualConfigs; /* current buffer */ for(conf = caps.configurations; conf; conf = conf->next) { for(stereo = 0; stereo < (conf->stereo ? 2 : 1); ++stereo) { @@ -137,7 +136,8 @@ __GLXconfig *__glXAquaCreateVisualConfigs(int *numConfigsPtr, int screenNumber) // Global c->visualID = -1; c->visualType = GLX_TRUE_COLOR; - c->next = c + 1; + c->next = calloc(sizeof(*visualConfigs), 1); + assert(c->next); c->level = 0; c->indexBits = 0; @@ -260,6 +260,7 @@ __GLXconfig *__glXAquaCreateVisualConfigs(int *numConfigsPtr, int screenNumber) /* EXT_framebuffer_sRGB */ c->sRGBCapable = GL_FALSE; + l = c; c = c->next; } } @@ -271,11 +272,8 @@ __GLXconfig *__glXAquaCreateVisualConfigs(int *numConfigsPtr, int screenNumber) } } - (c-1)->next = NULL; - - if (c - visualConfigs != numConfigs) { - FatalError("numConfigs calculation error in setVisualConfigs! numConfigs is %d i is %d\n", numConfigs, (int)(c - visualConfigs)); - } + free(c); + l->next = NULL; freeGlCapabilities(&caps); return visualConfigs;