From 9da6c0918f40359f28fe8889d5b7cae7efcc8377 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Sun, 29 Dec 2013 12:22:55 -0800 Subject: [PATCH 01/11] XQuartz: Silence some static analyzer warnings by annotating referencing counts Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/X11Application.m | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/hw/xquartz/X11Application.m b/hw/xquartz/X11Application.m index 1f9b05dd1..2efbd658b 100644 --- a/hw/xquartz/X11Application.m +++ b/hw/xquartz/X11Application.m @@ -70,6 +70,18 @@ xpbproxy_run(void); static dispatch_queue_t eventTranslationQueue; #endif +#ifndef __has_feature +#define __has_feature(x) 0 +#endif + +#ifndef CF_RETURNS_RETAINED +#if __has_feature(attribute_cf_returns_retained) +#define CF_RETURNS_RETAINED __attribute__((cf_returns_retained)) +#else +#define CF_RETURNS_RETAINED +#endif +#endif + extern Bool noTestExtensions; extern Bool noRenderExtension; extern BOOL serverRunning; @@ -526,6 +538,7 @@ cfrelease(CFAllocatorRef a, const void *b) CFRelease(b); } +CF_RETURNS_RETAINED static CFMutableArrayRef nsarray_to_cfarray(NSArray *in) { From 2e3ebec9520719a8e5c3c92390e83bcb5216f978 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Sun, 29 Dec 2013 12:31:23 -0800 Subject: [PATCH 02/11] XQuartz: Fix darwinfb.h header guard ./darwinfb.h:28:9: warning: '_DARWIN_FB_H' is used as a header guard here, followed by #define of a different macro [-Wheader-guard,Lexical or Preprocessor Issue] ^~~~~~~~~~~~ ./darwinfb.h:29:9: note: '_DARWIN_DB_H' is defined here; did you mean '_DARWIN_FB_H'? [Lexical or Preprocessor Issue] ^~~~~~~~~~~~ _DARWIN_FB_H Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/darwinfb.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xquartz/darwinfb.h b/hw/xquartz/darwinfb.h index 5de360d75..541128b8e 100644 --- a/hw/xquartz/darwinfb.h +++ b/hw/xquartz/darwinfb.h @@ -26,7 +26,7 @@ */ #ifndef _DARWIN_FB_H -#define _DARWIN_DB_H +#define _DARWIN_FB_H #include "scrnintstr.h" From ea80279e292e59a9fe9651489f03e9f2f39810d9 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Sun, 29 Dec 2013 12:36:51 -0800 Subject: [PATCH 03/11] XQuartz: Fix get_proc_address signature indirect.c:675:28: warning: incompatible pointer types passing 'glx_gpa_proc (*)(const char *)' to parameter of type 'glx_gpa_proc' (aka 'glx_func_ptr (*)(const char *)') [-Wincompatible-pointer-types,Semantic Issue] __glXsetGetProcAddress(&get_proc_address); ^~~~~~~~~~~~~~~~~ ../../../glx/glxserver.h:122:42: note: passing argument to parameter 'get_proc_address' here [Semantic Issue] void __glXsetGetProcAddress(glx_gpa_proc get_proc_address); ^ Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/GL/indirect.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/xquartz/GL/indirect.c b/hw/xquartz/GL/indirect.c index 8dabda14d..19b7d86e7 100644 --- a/hw/xquartz/GL/indirect.c +++ b/hw/xquartz/GL/indirect.c @@ -643,10 +643,10 @@ __glFloorLog2(GLuint val) static void *opengl_framework_handle; -static glx_gpa_proc +static glx_func_ptr get_proc_address(const char *sym) { - return (glx_gpa_proc) dlsym(opengl_framework_handle, sym); + return (glx_func_ptr) dlsym(opengl_framework_handle, sym); } static void From f79af1941776fd6f1ec26c50603fcc35ca7d514b Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Sun, 29 Dec 2013 12:41:18 -0800 Subject: [PATCH 04/11] XQuartz: Mark applicationWillTerminate: noreturn X11Controller.m:938:1: warning: method 'applicationWillTerminate:' could be declared with attribute 'noreturn' [-Wmissing-noreturn,Semantic Issue] { ^ Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/X11Controller.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xquartz/X11Controller.m b/hw/xquartz/X11Controller.m index 752bda35c..5445c6f3a 100644 --- a/hw/xquartz/X11Controller.m +++ b/hw/xquartz/X11Controller.m @@ -934,7 +934,7 @@ extern char *bundle_id_prefix; == NSAlertDefaultReturn) ? NSTerminateNow : NSTerminateCancel; } -- (void) applicationWillTerminate:(NSNotification *)aNotification +- (void) applicationWillTerminate:(NSNotification *)aNotification _X_NORETURN { int remain; [X11App prefs_synchronize]; From 959e8f23af7850fcaf40d6c67f5228241a36a9ab Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Sun, 29 Dec 2013 12:45:23 -0800 Subject: [PATCH 05/11] XQuartz: Simplify hook_run to quiet static analyzer x-hook.c:96:9: warning: Called function pointer is an uninitalized pointer value (*fun[i])(arg, data[i]); ^~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/xpr/x-hook.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/hw/xquartz/xpr/x-hook.c b/hw/xquartz/xpr/x-hook.c index b5d8ab90e..3922bb86c 100644 --- a/hw/xquartz/xpr/x-hook.c +++ b/hw/xquartz/xpr/x-hook.c @@ -70,34 +70,19 @@ X_PFX(hook_remove) (x_list * lst, x_hook_function * fun, void *data) { X_EXTERN void X_PFX(hook_run) (x_list * lst, void *arg) { - x_list *node, *cell; - x_hook_function **fun; - void **data; - int length, i; + x_list *node; if (!lst) return; - length = X_PFX(list_length) (lst); - fun = malloc(sizeof(x_hook_function *) * length); - data = malloc(sizeof(void *) * length); + for (node = lst; node != NULL; node = node->next) { + x_list *cell = node->data; - if (!fun || !data) { - FatalError("Failed to allocate memory in %s\n", __func__); + x_hook_function *fun = CELL_FUN(cell); + void *data = CELL_DATA(cell); + + (*fun)(arg, data); } - - for (i = 0, node = lst; node != NULL; node = node->next, i++) { - cell = node->data; - fun[i] = CELL_FUN(cell); - data[i] = CELL_DATA(cell); - } - - for (i = 0; i < length; i++) { - (*fun[i])(arg, data[i]); - } - - free(fun); - free(data); } X_EXTERN void From b3572c0d1ab7888ac26d6b2b8be6d1d19ed9af3f Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Wed, 1 Jan 2014 10:39:56 -0800 Subject: [PATCH 06/11] XQuartz: Validate screen in AppleDRIQueryDirectRenderingCapable requests Return an error to the caller rather than crashing the server on invalid screens. Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/xpr/appledri.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/xquartz/xpr/appledri.c b/hw/xquartz/xpr/appledri.c index 9aac07240..d7e984467 100644 --- a/hw/xquartz/xpr/appledri.c +++ b/hw/xquartz/xpr/appledri.c @@ -123,6 +123,10 @@ ProcAppleDRIQueryDirectRenderingCapable(register ClientPtr client) rep.length = 0; rep.sequenceNumber = client->sequence; + if (stuff->screen >= screenInfo.numScreens) { + return BadValue; + } + if (!DRIQueryDirectRenderingCapable(screenInfo.screens[stuff->screen], &isCapable)) { return BadValue; From a03f096a85537d9e881cedaa6cb71aca43a97086 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Wed, 1 Jan 2014 10:47:52 -0800 Subject: [PATCH 07/11] XQuartz: Validate length in appledri before swapping Avoids potential memory corruption from bad requests Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/xpr/appledri.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/xquartz/xpr/appledri.c b/hw/xquartz/xpr/appledri.c index d7e984467..77574655b 100644 --- a/hw/xquartz/xpr/appledri.c +++ b/hw/xquartz/xpr/appledri.c @@ -406,6 +406,7 @@ SProcAppleDRIQueryDirectRenderingCapable(register ClientPtr client) { REQUEST(xAppleDRIQueryDirectRenderingCapableReq); swaps(&stuff->length); + REQUEST_SIZE_MATCH(xAppleDRIQueryDirectRenderingCapableReq); swapl(&stuff->screen); return ProcAppleDRIQueryDirectRenderingCapable(client); } @@ -415,6 +416,7 @@ SProcAppleDRIAuthConnection(register ClientPtr client) { REQUEST(xAppleDRIAuthConnectionReq); swaps(&stuff->length); + REQUEST_SIZE_MATCH(xAppleDRIAuthConnectionReq); swapl(&stuff->screen); swapl(&stuff->magic); return ProcAppleDRIAuthConnection(client); @@ -425,6 +427,7 @@ SProcAppleDRICreateSurface(register ClientPtr client) { REQUEST(xAppleDRICreateSurfaceReq); swaps(&stuff->length); + REQUEST_SIZE_MATCH(xAppleDRICreateSurfaceReq); swapl(&stuff->screen); swapl(&stuff->drawable); swapl(&stuff->client_id); @@ -436,6 +439,7 @@ SProcAppleDRIDestroySurface(register ClientPtr client) { REQUEST(xAppleDRIDestroySurfaceReq); swaps(&stuff->length); + REQUEST_SIZE_MATCH(xAppleDRIDestroySurfaceReq); swapl(&stuff->screen); swapl(&stuff->drawable); return ProcAppleDRIDestroySurface(client); @@ -446,6 +450,7 @@ SProcAppleDRICreatePixmap(register ClientPtr client) { REQUEST(xAppleDRICreatePixmapReq); swaps(&stuff->length); + REQUEST_SIZE_MATCH(xAppleDRICreatePixmapReq); swapl(&stuff->screen); swapl(&stuff->drawable); return ProcAppleDRICreatePixmap(client); @@ -456,6 +461,7 @@ SProcAppleDRIDestroyPixmap(register ClientPtr client) { REQUEST(xAppleDRIDestroyPixmapReq); swaps(&stuff->length); + REQUEST_SIZE_MATCH(xAppleDRIDestroyPixmapReq); swapl(&stuff->drawable); return ProcAppleDRIDestroyPixmap(client); } From b2f6b3497c33a4897afae80a2cf69c596b9f81e8 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Wed, 1 Jan 2014 10:55:10 -0800 Subject: [PATCH 08/11] XQuartz: Silence a clang static analysis warning about a possible memory leak on exit stub.c:356:9: warning: Potential leak of memory pointed to by 'newargv' asl_log(aslc, NULL, ASL_LEVEL_ERR, ^~~~~~~ stub.c:356:9: warning: Potential leak of memory pointed to by 'newenvp' asl_log(aslc, NULL, ASL_LEVEL_ERR, ^~~~~~~ 2 warnings generated. Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/mach-startup/stub.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/xquartz/mach-startup/stub.c b/hw/xquartz/mach-startup/stub.c index b5a3168ca..756e4ef2d 100644 --- a/hw/xquartz/mach-startup/stub.c +++ b/hw/xquartz/mach-startup/stub.c @@ -353,6 +353,10 @@ main(int argc, char **argv, char **envp) newenvp = (string_array_t)calloc((1 + envpc), sizeof(string_t)); if (!newargv || !newenvp) { + /* Silence the clang static analyzer */ + free(newargv); + free(newenvp); + asl_log(aslc, NULL, ASL_LEVEL_ERR, "Xquartz: Memory allocation failure"); return EXIT_FAILURE; From 64327226ddfba8f0653615cd678d2d4336fb993d Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Wed, 1 Jan 2014 11:00:40 -0800 Subject: [PATCH 09/11] XQuartz: Silence a clang static analysis warning about a memory leak It seems the alanyzer can't comprehend dixSetPrivate(). quartz.c:119:12: warning: Potential leak of memory pointed to by 'displayInfo' return quartzProcs->AddScreen(index, pScreen); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/quartz.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/xquartz/quartz.c b/hw/xquartz/quartz.c index 5b977c7f9..bc6c8d048 100644 --- a/hw/xquartz/quartz.c +++ b/hw/xquartz/quartz.c @@ -109,11 +109,14 @@ Bool QuartzAddScreen(int index, ScreenPtr pScreen) { + // The clang static analyzer thinks we leak displayInfo here +#ifndef __clang_analyzer__ // allocate space for private per screen Quartz specific storage QuartzScreenPtr displayInfo = calloc(sizeof(QuartzScreenRec), 1); // QUARTZ_PRIV(pScreen) = displayInfo; dixSetPrivate(&pScreen->devPrivates, quartzScreenKey, displayInfo); +#endif /* __clang_analyzer__ */ // do Quartz mode specific initialization return quartzProcs->AddScreen(index, pScreen); From 3bc608a361a01043b226fb9aaebf88f6fd852925 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Wed, 1 Jan 2014 11:04:07 -0800 Subject: [PATCH 10/11] XQuartz: Check for allocated memory before using it in AppleWMSetWindowMenu Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/applewm.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/xquartz/applewm.c b/hw/xquartz/applewm.c index 4409d4bf6..9ff0ff650 100644 --- a/hw/xquartz/applewm.c +++ b/hw/xquartz/applewm.c @@ -378,6 +378,13 @@ ProcAppleWMSetWindowMenu(register ClientPtr client) items = malloc(sizeof(char *) * nitems); shortcuts = malloc(sizeof(char) * nitems); + if (!items || !shortcuts) { + free(items); + free(shortcuts); + + return BadAlloc; + } + max_len = (stuff->length << 2) - sizeof(xAppleWMSetWindowMenuReq); bytes = (char *)&stuff[1]; From 77df653ae3d8448be21221711851acde12c6bc1a Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston Sequoia Date: Wed, 1 Jan 2014 11:10:41 -0800 Subject: [PATCH 11/11] XQuartz: Avoid passing uninitialized pointers to X11ApplicationSetWindowMenu in AppleWMSetWindowMenu Signed-off-by: Jeremy Huddleston Sequoia --- hw/xquartz/applewm.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hw/xquartz/applewm.c b/hw/xquartz/applewm.c index 9ff0ff650..cc91c9608 100644 --- a/hw/xquartz/applewm.c +++ b/hw/xquartz/applewm.c @@ -398,6 +398,15 @@ ProcAppleWMSetWindowMenu(register ClientPtr client) break; } } + + /* Check if we bailed out of the above loop due to a request that was too long */ + if (j < nitems) { + free(items); + free(shortcuts); + + return BadRequest; + } + X11ApplicationSetWindowMenu(nitems, items, shortcuts); free(items); free(shortcuts);