From 1a72bc41460aa37b2ae6ce37bc9fb9e3644f5943 Mon Sep 17 00:00:00 2001 From: Pauli Nieminen Date: Mon, 3 Jan 2011 12:25:28 -0500 Subject: Initialize event type If we receive unsupported event closing connection triggers valgrind error. ==12017== Conditional jump or move depends on uninitialised value(s) ==12017== at 0x487D454: _XFreeDisplayStructure (OpenDis.c:607) ==12017== by 0x486857B: XCloseDisplay (ClDisplay.c:72) *snip* ==12017== Uninitialised value was created by a heap allocation ==12017== at 0x4834C48: malloc (vg_replace_malloc.c:236) ==12017== by 0x4894147: _XEnq (XlibInt.c:877) ==12017== by 0x4891BF3: handle_response (xcb_io.c:335) ==12017== by 0x4892263: _XReply (xcb_io.c:626) *snip* Problem is that XFreeDisplaySturture is checking for qelt->event.type == GenericEvent while _XUnknownWireEvent doesn't store the type. Reviewed-by: Adam Jackson Reviewed-by: Peter Hutterer Signed-off-by: Pauli Nieminen Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 7ca1d0d01..e4078ca31 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -2951,6 +2951,8 @@ void _XEnq( type = event->u.u.type & 0177; extension = ((xGenericEvent*)event)->extension; + + qelt->event.type = type; /* If an extension has registerd a generic_event_vec handler, then * it can handle event cookies. Otherwise, proceed with the normal * event handlers. -- cgit v1.2.3 From 8aacb3fab5824e20e07699cef93c5a20daaff368 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erkki=20Sepp=C3=A4l=C3=A4?= Date: Mon, 10 Jan 2011 16:17:47 +0200 Subject: Using freed pointer "e" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reordered code to first to do the comparison and then to release data Reviewed-by: Alan Coopersmith Reviewed-by: Ander Conselvan de Oliveira Signed-off-by: Erkki Seppälä Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index e4078ca31..917f503fa 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -2839,10 +2839,10 @@ _XFreeEventCookies(Display *dpy) head = (struct stored_event**)&dpy->cookiejar; DL_FOREACH_SAFE(*head, e, tmp) { - XFree(e->ev.data); - XFree(e); if (dpy->cookiejar == e) dpy->cookiejar = NULL; + XFree(e->ev.data); + XFree(e); } } -- cgit v1.2.3 From 698270c47b3b78e3e746c4767a277c5e1e73ec14 Mon Sep 17 00:00:00 2001 From: Ander Conselvan de Oliveira Date: Mon, 31 Jan 2011 14:02:12 +0200 Subject: XlibInt: info_list->watch_data was reallocated, but result was discarded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit info_list->watch_data was being reallocated, but the return value of the reallocation was stored only into a local variable. This might cause some funky behavior and crashes. Variable "wd_array" goes out of scope Value "wd_array" is overwritten in "wd_array = (XPointer*)realloc((char*)info_list->watch_data, (((dpy->watcher_count + 1) * 4U == 0U) ? 1U : ((dpy->watcher_count + 1) * 4U)))" Reviewed-by: Alan Coopersmith Reviewed-by: Erkki Seppälä Signed-off-by: Ander Conselvan de Oliveira Signed-off-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 1 + 1 file changed, 1 insertion(+) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 917f503fa..23717fd7f 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -2673,6 +2673,7 @@ XAddConnectionWatch( UnlockDisplay(dpy); return 0; } + info_list->watch_data = wd_array; wd_array[dpy->watcher_count] = NULL; /* for cleanliness */ } -- cgit v1.2.3 From 20470a83e3a57f936cd90856325b897fa115b9ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erkki=20Sepp=C3=A4l=C3=A4?= Date: Mon, 31 Jan 2011 14:01:57 +0200 Subject: XlibInt: Use strncpy+zero termination instead of strcpy to enforce buffer size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Possible overrun of 8192 byte fixed size buffer "buffer" by copying "ext->name" without length checking Reviewed-by: Alan Coopersmith Reviewed-by: Ander Conselvan de Oliveira Signed-off-by: Erkki Seppälä Signed-off-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 23717fd7f..151e521bd 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -3528,9 +3528,10 @@ static int _XPrintDefaultError( ext && (ext->codes.major_opcode != event->request_code); ext = ext->next) ; - if (ext) - strcpy(buffer, ext->name); - else + if (ext) { + strncpy(buffer, ext->name, BUFSIZ); + buffer[BUFSIZ - 1] = '\0'; + } else buffer[0] = '\0'; } (void) fprintf(fp, " (%s)\n", buffer); -- cgit v1.2.3 From 79a5330db17102dd62506b6d748244acb2074369 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 30 Apr 2012 16:36:47 +1000 Subject: Typo fix Signed-off-by: Peter Hutterer Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 151e521bd..8dc07ad1b 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -2954,7 +2954,7 @@ void _XEnq( extension = ((xGenericEvent*)event)->extension; qelt->event.type = type; - /* If an extension has registerd a generic_event_vec handler, then + /* If an extension has registered a generic_event_vec handler, then * it can handle event cookies. Otherwise, proceed with the normal * event handlers. * -- cgit v1.2.3 From 5e0584c43d3511a5544246a0f82c759ce860a0c2 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Fri, 15 Feb 2013 23:34:40 -0800 Subject: Preserve constness in casting arguments through the Data*() routines Casts were annoying gcc by dropping constness when changing types, when routines simply either copy data into the request buffer or send it directly to the X server, and never modify the input. Fixes gcc warnings including: ChProp.c: In function 'XChangeProperty': ChProp.c:65:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] ChProp.c:65:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] ChProp.c:74:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] ChProp.c:74:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] ChProp.c:83:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] SetHints.c: In function 'XSetStandardProperties': SetHints.c:262:20: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] SetPntMap.c: In function 'XSetPointerMapping': SetPntMap.c:46:5: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] SetPntMap.c:46:5: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] StBytes.c: In function 'XStoreBuffer': StBytes.c:97:33: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] StName.c: In function 'XStoreName': StName.c:40:27: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] StName.c: In function 'XSetIconName': StName.c:51:27: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual] Signed-off-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 8dc07ad1b..2d84c626d 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -3852,7 +3852,7 @@ void _Xbcopy(b1, b2, length) #ifdef DataRoutineIsProcedure void Data( Display *dpy, - char *data, + _Xconst char *data, long len) { if (dpy->bufptr + (len) <= dpy->bufmax) { @@ -3869,7 +3869,7 @@ void Data( int _XData32( Display *dpy, - register long *data, + register _Xconst long *data, unsigned len) { register int *buf; -- cgit v1.2.3 From d31b81c1010b4259bb0caafdbd82483fabda787d Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Sat, 16 Feb 2013 10:42:23 -0800 Subject: Convert more sprintf calls to snprintf You could analyze most of these and quickly recognize that there was no chance of buffer overflow already, but why make everyone spend time doing that when we can just make it obviously safe? Signed-off-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 2d84c626d..1ecbaaa4f 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -3521,7 +3521,7 @@ static int _XPrintDefaultError( mesg, BUFSIZ); (void) fprintf(fp, mesg, event->request_code); if (event->request_code < 128) { - sprintf(number, "%d", event->request_code); + snprintf(number, sizeof(number), "%d", event->request_code); XGetErrorDatabaseText(dpy, "XRequest", number, "", buffer, BUFSIZ); } else { for (ext = dpy->ext_procs; @@ -3541,7 +3541,7 @@ static int _XPrintDefaultError( fputs(" ", fp); (void) fprintf(fp, mesg, event->minor_code); if (ext) { - sprintf(mesg, "%s.%d", ext->name, event->minor_code); + snprintf(mesg, sizeof(mesg), "%s.%d", ext->name, event->minor_code); XGetErrorDatabaseText(dpy, "XRequest", mesg, "", buffer, BUFSIZ); (void) fprintf(fp, " (%s)", buffer); } @@ -3564,8 +3564,8 @@ static int _XPrintDefaultError( bext = ext; } if (bext) - sprintf(buffer, "%s.%d", bext->name, - event->error_code - bext->codes.first_error); + snprintf(buffer, sizeof(buffer), "%s.%d", bext->name, + event->error_code - bext->codes.first_error); else strcpy(buffer, "Value"); XGetErrorDatabaseText(dpy, mtype, buffer, "", mesg, BUFSIZ); -- cgit v1.2.3 From e15023b2ad2a5ca8742cbb93fe10cb38ab079831 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Thu, 7 Mar 2013 23:46:05 -0800 Subject: Remove more unnecessary casts from Xmalloc/calloc calls Signed-off-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 1ecbaaa4f..4c4e80e6a 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -608,7 +608,7 @@ Bool _XPollfdCacheInit( #ifdef USE_POLL struct pollfd *pfp; - pfp = (struct pollfd *)Xmalloc(POLLFD_CACHE_SIZE * sizeof(struct pollfd)); + pfp = Xmalloc(POLLFD_CACHE_SIZE * sizeof(struct pollfd)); if (!pfp) return False; pfp[0].fd = dpy->fd; @@ -2477,10 +2477,10 @@ _XRegisterInternalConnection( #if defined(NX_TRANS_SOCKET) && defined(NX_TRANS_DEBUG) fprintf(stderr, "_XRegisterInternalConnection: Got called.\n"); #endif - new_conni = (struct _XConnectionInfo*)Xmalloc(sizeof(struct _XConnectionInfo)); + new_conni = Xmalloc(sizeof(struct _XConnectionInfo)); if (!new_conni) return 0; - new_conni->watch_data = (XPointer *)Xmalloc(dpy->watcher_count * sizeof(XPointer)); + new_conni->watch_data = Xmalloc(dpy->watcher_count * sizeof(XPointer)); if (!new_conni->watch_data) { Xfree(new_conni); return 0; @@ -2573,7 +2573,7 @@ XInternalConnectionNumbers( count = 0; for (info_list=dpy->im_fd_info; info_list; info_list=info_list->next) count++; - fd_list = (int*) Xmalloc (count * sizeof(int)); + fd_list = Xmalloc (count * sizeof(int)); if (!fd_list) { UnlockDisplay(dpy); return 0; @@ -2666,9 +2666,8 @@ XAddConnectionWatch( /* allocate new watch data */ for (info_list=dpy->im_fd_info; info_list; info_list=info_list->next) { - wd_array = (XPointer *)Xrealloc((char *)info_list->watch_data, - (dpy->watcher_count + 1) * - sizeof(XPointer)); + wd_array = Xrealloc(info_list->watch_data, + (dpy->watcher_count + 1) * sizeof(XPointer)); if (!wd_array) { UnlockDisplay(dpy); return 0; @@ -2677,7 +2676,7 @@ XAddConnectionWatch( wd_array[dpy->watcher_count] = NULL; /* for cleanliness */ } - new_watcher = (struct _XConnWatchInfo*)Xmalloc(sizeof(struct _XConnWatchInfo)); + new_watcher = Xmalloc(sizeof(struct _XConnWatchInfo)); if (!new_watcher) { UnlockDisplay(dpy); return 0; @@ -2936,8 +2935,7 @@ void _XEnq( /* If dpy->qfree is non-NULL do this, else malloc a new one. */ dpy->qfree = qelt->next; } - else if ((qelt = - (_XQEvent *) Xmalloc((unsigned)sizeof(_XQEvent))) == NULL) { + else if ((qelt = Xmalloc(sizeof(_XQEvent))) == NULL) { /* Malloc call failed! */ ESET(ENOMEM); #ifdef NX_TRANS_SOCKET @@ -3766,8 +3764,8 @@ char *_XAllocScratch( { if (nbytes > dpy->scratch_length) { if (dpy->scratch_buffer) Xfree (dpy->scratch_buffer); - if ((dpy->scratch_buffer = Xmalloc((unsigned) nbytes))) - dpy->scratch_length = nbytes; + if ((dpy->scratch_buffer = Xmalloc(nbytes))) + dpy->scratch_length = nbytes; else dpy->scratch_length = 0; } return (dpy->scratch_buffer); -- cgit v1.2.3 From 61fdf93ef56818c081fb775a17fdc513315957c3 Mon Sep 17 00:00:00 2001 From: Thomas Klausner Date: Tue, 25 Jun 2013 18:33:56 +0200 Subject: Check for symbol existence with #ifdef, not #if Reviewed-by: Jamey Sharp Signed-off-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 4c4e80e6a..564d318fc 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -901,7 +901,7 @@ void _XSeqSyncFunction( static int _XPrivSyncFunction (Display *dpy) { -#if XTHREADS +#ifdef XTHREADS assert(!dpy->lock_fns); #endif assert(dpy->synchandler == _XPrivSyncFunction); -- cgit v1.2.3 From ea8239650656846d810a93e7659e182e412f07b3 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Sun, 9 Jun 2013 11:13:42 -0700 Subject: libX11: check size of GetReqExtra after XFlush Two users of GetReqExtra pass arbitrarily sized allocations from the caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra macro) to double-check the requested length and invalidate "req" when this happens. Users of GetReqExtra passing lengths greater than the Xlib buffer size (normally 16K) must check "req" and fail gracefully instead of crashing. Any callers of GetReqExtra that do not check "req" for NULL will experience this change, in the pathological case, as a NULL dereference instead of a buffer overflow. This is an improvement, but the documentation for GetReqExtra has been updated to reflect the need to check the value of "req" after the call. Bug that manifested the problem: https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628 Signed-off-by: Kees Cook Reviewed-by: Alan Coopersmith Signed-off-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 564d318fc..deab8b3ad 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -3980,6 +3980,14 @@ void *_XGetRequest(Display *dpy, CARD8 type, size_t len) if (dpy->bufptr + len > dpy->bufmax) _XFlush(dpy); + /* Request still too large, so do not allow it to overflow. */ + if (dpy->bufptr + len > dpy->bufmax) { + fprintf(stderr, + "Xlib: request %d length %zd would exceed buffer size.\n", + type, len); + /* Changes failure condition from overflow to NULL dereference. */ + return NULL; + } if (len % 4) fprintf(stderr, -- cgit v1.2.3 From fc26b97ea9053a2aba54824243282e27bc4a1e15 Mon Sep 17 00:00:00 2001 From: walter harms Date: Thu, 5 Jun 2014 18:37:40 +0200 Subject: Remove redundant null checks before free This patch removes some redundant null checks before free. It should not change the code otherwise. Be aware that this is only the first series. Signed-off-by: Harms Reviewed-by: Alan Coopersmith Signed-off-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index deab8b3ad..418853391 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -2537,8 +2537,7 @@ _XUnregisterInternalConnection( watch=watch->next, wd++) { (*watch->fn) (dpy, watch->client_data, fd, False, wd); } - if (info_list->watch_data) - Xfree (info_list->watch_data); + Xfree (info_list->watch_data); Xfree (info_list); break; } @@ -3763,9 +3762,10 @@ char *_XAllocScratch( unsigned long nbytes) { if (nbytes > dpy->scratch_length) { - if (dpy->scratch_buffer) Xfree (dpy->scratch_buffer); - if ((dpy->scratch_buffer = Xmalloc(nbytes))) - dpy->scratch_length = nbytes; + Xfree (dpy->scratch_buffer); + dpy->scratch_buffer = Xmalloc(nbytes); + if (dpy->scratch_buffer) + dpy->scratch_length = nbytes; else dpy->scratch_length = 0; } return (dpy->scratch_buffer); @@ -3792,8 +3792,8 @@ void _XFreeTemp( char *buf, unsigned long nbytes) { - if (dpy->scratch_buffer) - Xfree(dpy->scratch_buffer); + + Xfree(dpy->scratch_buffer); dpy->scratch_buffer = buf; dpy->scratch_length = nbytes; } -- cgit v1.2.3 From fc524ddbba875a5f091f13aaf5d851c7afb299f4 Mon Sep 17 00:00:00 2001 From: Thomas Klausner Date: Sun, 19 Jul 2015 10:22:45 +0200 Subject: Do not return() after exit(). Signed-off-by: Thomas Klausner Reviewed-by: Alan Coopersmith Backported-to-NX-by: Ulrich Sibiller --- nx-X11/lib/X11/XlibInt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'nx-X11/lib/X11/XlibInt.c') diff --git a/nx-X11/lib/X11/XlibInt.c b/nx-X11/lib/X11/XlibInt.c index 418853391..d3c85a343 100644 --- a/nx-X11/lib/X11/XlibInt.c +++ b/nx-X11/lib/X11/XlibInt.c @@ -3496,7 +3496,7 @@ int _XDefaultIOError( #else exit(1); #endif /* #ifdef NX_TRANS_SOCKET */ - return(0); /* dummy - function should never return */ + /*NOTREACHED*/ } @@ -3746,7 +3746,7 @@ _XIOError ( #else exit (1); #endif - return 0; + /*NOTREACHED*/ } -- cgit v1.2.3