From 1c900f7635a542336dbb70b9f160df004eaf6317 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 25 Aug 2022 14:59:57 -0400 Subject: [PATCH] Add bounds-checking variants of reply functions and fix locking Deprecate the unsafe functions. Signed-off-by: Demi Marie Obenour --- src/c_client.py | 5 +- src/xcb_in.c | 176 +++++++++++++++++++++++++++++++++--------------- src/xcbext.h | 79 +++++++++++++++++++++- 3 files changed, 202 insertions(+), 58 deletions(-) diff --git a/src/c_client.py b/src/c_client.py index b7db543..6452012 100644 --- a/src/c_client.py +++ b/src/c_client.py @@ -2569,9 +2569,10 @@ def _c_reply(self, name): if len(unserialize_fields)>0: # certain variable size fields need to be unserialized explicitly - _c(' %s *reply = (%s *) xcb_wait_for_reply(c, cookie.sequence, e);', + _c(' %s *reply = (%s *) xcb_wait_for_reply_safe(c, cookie.sequence, e, sizeof (*reply));', self.c_reply_type, self.c_reply_type) _c(' int i;') + for field in unserialize_fields: if field.type.is_list: _c(' %s %s_iter = %s(reply);', field.c_iterator_type, field.c_field_name, field.c_iterator_name) @@ -2593,7 +2594,7 @@ def _c_reply(self, name): _c(' return reply;') else: - _c(' return (%s *) xcb_wait_for_reply(c, cookie.sequence, e);', self.c_reply_type) + _c(' return (%s *) xcb_wait_for_reply_safe(c, cookie.sequence, e, sizeof (%s));', self.c_reply_type, self.c_reply_type) _c('}') diff --git a/src/xcb_in.c b/src/xcb_in.c index c9a264d..fcbd068 100644 --- a/src/xcb_in.c +++ b/src/xcb_in.c @@ -34,6 +34,7 @@ #include #include #include +#include #if USE_POLL #include @@ -251,7 +252,7 @@ static int read_packet(xcb_connection_t *c) /* XXX a bit of a hack -- we "know" that all FD replys place * the number of fds in the pad0 byte */ - if (pend && pend->flags & XCB_REQUEST_REPLY_FDS) + if (pend && (pend->flags & XCB_REQUEST_REPLY_FDS)) nfd = genrep.pad0; } @@ -261,7 +262,7 @@ static int read_packet(xcb_connection_t *c) bufsize = length + eventlength + nfd * sizeof(int) + (genrep.response_type == XCB_REPLY ? 0 : sizeof(uint32_t)); - if (bufsize < INT32_MAX) + if (bufsize < INT32_MAX && length <= INT_MAX && eventlength <= INT_MAX) buf = malloc((size_t) bufsize); else buf = NULL; @@ -510,6 +511,12 @@ static void *wait_for_reply(xcb_connection_t *c, uint64_t request, xcb_generic_e { void *ret = 0; + if (e) + *e = NULL; + + if (c->has_error) + return NULL; + /* If this request has not been written yet, write it. */ if(c->out.return_socket || _xcb_out_flush_to(c, request)) { @@ -538,16 +545,24 @@ static uint64_t widen(xcb_connection_t *c, unsigned int request) return widened_request; } +static void *wait_for_reply_safe(xcb_connection_t *c, uint64_t request, xcb_generic_error_t **e, size_t size) +{ + xcb_generic_reply_t *ret = NULL; + ret = wait_for_reply(c, request, e); + if (ret && size > 32 && ret->length < ((size - 29) >> 2)) + { + _xcb_conn_shutdown(c, XCB_CONN_CLOSED_PARSE_ERR); + free(ret); + ret = NULL; + } + return ret; +} + /* Public interface */ void *xcb_wait_for_reply(xcb_connection_t *c, unsigned int request, xcb_generic_error_t **e) { void *ret; - if(e) - *e = 0; - if(c->has_error) - return 0; - pthread_mutex_lock(&c->iolock); ret = wait_for_reply(c, widen(c, request), e); pthread_mutex_unlock(&c->iolock); @@ -557,17 +572,30 @@ void *xcb_wait_for_reply(xcb_connection_t *c, unsigned int request, xcb_generic_ void *xcb_wait_for_reply64(xcb_connection_t *c, uint64_t request, xcb_generic_error_t **e) { void *ret; - if(e) - *e = 0; - if(c->has_error) - return 0; - pthread_mutex_lock(&c->iolock); ret = wait_for_reply(c, request, e); pthread_mutex_unlock(&c->iolock); return ret; } +void *xcb_wait_for_reply_safe(xcb_connection_t *c, unsigned int request, xcb_generic_error_t **e, size_t size) +{ + xcb_generic_reply_t *ret = NULL; + pthread_mutex_lock(&c->iolock); + ret = wait_for_reply_safe(c, widen(c, request), e, size); + pthread_mutex_unlock(&c->iolock); + return ret; +} + +void *xcb_wait_for_reply64_safe(xcb_connection_t *c, uint64_t request, xcb_generic_error_t **e, size_t size) +{ + xcb_generic_reply_t *ret = NULL; + pthread_mutex_lock(&c->iolock); + ret = wait_for_reply_safe(c, request, e, size); + pthread_mutex_unlock(&c->iolock); + return ret; +} + int *xcb_get_reply_fds(xcb_connection_t *c, void *reply, size_t reply_size) { return (int *) (&((char *) reply)[reply_size]); @@ -599,6 +627,9 @@ static void discard_reply(xcb_connection_t *c, uint64_t request) void *reply; pending_reply **prev_pend; + if (c->has_error) + return; + /* Free any replies or errors that we've already read. Stop if * xcb_wait_for_reply would block or we've run out of replies. */ while(poll_for_reply(c, request, &reply, 0) && reply) @@ -628,9 +659,6 @@ static void discard_reply(xcb_connection_t *c, uint64_t request) void xcb_discard_reply(xcb_connection_t *c, unsigned int sequence) { - if(c->has_error) - return; - /* If an error occurred when issuing the request, fail immediately. */ if(!sequence) return; @@ -642,9 +670,6 @@ void xcb_discard_reply(xcb_connection_t *c, unsigned int sequence) void xcb_discard_reply64(xcb_connection_t *c, uint64_t sequence) { - if(c->has_error) - return; - /* If an error occurred when issuing the request, fail immediately. */ if(!sequence) return; @@ -654,9 +679,10 @@ void xcb_discard_reply64(xcb_connection_t *c, uint64_t sequence) pthread_mutex_unlock(&c->iolock); } -int xcb_poll_for_reply(xcb_connection_t *c, unsigned int request, void **reply, xcb_generic_error_t **error) +static int poll_for_reply_inner(xcb_connection_t *c, uint64_t request, void **reply, xcb_generic_error_t **error) { int ret; + assert(reply != 0); if(c->has_error) { *reply = 0; @@ -664,11 +690,33 @@ int xcb_poll_for_reply(xcb_connection_t *c, unsigned int request, void **reply, *error = 0; return 1; /* would not block */ } - assert(reply != 0); - pthread_mutex_lock(&c->iolock); - ret = poll_for_reply(c, widen(c, request), reply, error); + ret = poll_for_reply(c, request, reply, error); if(!ret && c->in.reading == 0 && _xcb_in_read(c)) /* _xcb_in_read shuts down the connection on error */ - ret = poll_for_reply(c, widen(c, request), reply, error); + ret = poll_for_reply(c, request, reply, error); + return ret; +} + +static int poll_for_reply_inner_safe(xcb_connection_t *c, uint64_t request, void **reply, xcb_generic_error_t **error, size_t size) +{ + int ret = poll_for_reply_inner(c, request, reply, error); + if (ret && *reply) + { + xcb_generic_reply_t *genrep = *reply; + if (size > 32 && genrep->length < ((size - 29) >> 2)) + { + _xcb_conn_shutdown(c, XCB_CONN_CLOSED_PARSE_ERR); + free(genrep); + *reply = NULL; + } + } + return ret; +} + +int xcb_poll_for_reply(xcb_connection_t *c, unsigned int request, void **reply, xcb_generic_error_t **error) +{ + int ret; + pthread_mutex_lock(&c->iolock); + ret = poll_for_reply_inner(c, widen(c, request), reply, error); pthread_mutex_unlock(&c->iolock); return ret; } @@ -676,34 +724,43 @@ int xcb_poll_for_reply(xcb_connection_t *c, unsigned int request, void **reply, int xcb_poll_for_reply64(xcb_connection_t *c, uint64_t request, void **reply, xcb_generic_error_t **error) { int ret; - if(c->has_error) - { - *reply = 0; - if(error) - *error = 0; - return 1; /* would not block */ - } - assert(reply != 0); pthread_mutex_lock(&c->iolock); - ret = poll_for_reply(c, request, reply, error); - if(!ret && c->in.reading == 0 && _xcb_in_read(c)) /* _xcb_in_read shuts down the connection on error */ - ret = poll_for_reply(c, request, reply, error); + ret = poll_for_reply_inner(c, request, reply, error); + pthread_mutex_unlock(&c->iolock); + return ret; +} + +int xcb_poll_for_reply_safe(xcb_connection_t *c, unsigned int request, void **reply, xcb_generic_error_t **error, size_t size) +{ + int ret; + pthread_mutex_lock(&c->iolock); + ret = poll_for_reply_inner_safe(c, widen(c, request), reply, error, size); + pthread_mutex_unlock(&c->iolock); + return ret; +} + +int xcb_poll_for_reply64_safe(xcb_connection_t *c, uint64_t request, void **reply, xcb_generic_error_t **error, size_t size) +{ + int ret; + pthread_mutex_lock(&c->iolock); + ret = poll_for_reply_inner_safe(c, request, reply, error, size); pthread_mutex_unlock(&c->iolock); return ret; } xcb_generic_event_t *xcb_wait_for_event(xcb_connection_t *c) { - xcb_generic_event_t *ret; - if(c->has_error) - return 0; + xcb_generic_event_t *ret = NULL; pthread_mutex_lock(&c->iolock); - /* get_event returns 0 on empty list. */ - while(!(ret = get_event(c))) - if(!_xcb_conn_wait(c, &c->in.event_cond, 0, 0)) - break; + if(!c->has_error) + { + /* get_event returns 0 on empty list. */ + while(!(ret = get_event(c))) + if(!_xcb_conn_wait(c, &c->in.event_cond, 0, 0)) + break; - _xcb_in_wake_up_next_reader(c); + _xcb_in_wake_up_next_reader(c); + } pthread_mutex_unlock(&c->iolock); return ret; } @@ -711,15 +768,15 @@ xcb_generic_event_t *xcb_wait_for_event(xcb_connection_t *c) static xcb_generic_event_t *poll_for_next_event(xcb_connection_t *c, int queued) { xcb_generic_event_t *ret = 0; + pthread_mutex_lock(&c->iolock); if(!c->has_error) { - pthread_mutex_lock(&c->iolock); /* FIXME: follow X meets Z architecture changes. */ ret = get_event(c); if(!ret && !queued && c->in.reading == 0 && _xcb_in_read(c)) /* _xcb_in_read shuts down the connection on error */ ret = get_event(c); - pthread_mutex_unlock(&c->iolock); } + pthread_mutex_unlock(&c->iolock); return ret; } @@ -738,9 +795,11 @@ xcb_generic_error_t *xcb_request_check(xcb_connection_t *c, xcb_void_cookie_t co uint64_t request; xcb_generic_error_t *ret = 0; void *reply; - if(c->has_error) - return 0; pthread_mutex_lock(&c->iolock); + if(c->has_error) { + pthread_mutex_unlock(&c->iolock); + return 0; + } request = widen(c, cookie.sequence); if (XCB_SEQUENCE_COMPARE(request, >, c->in.request_completed)) { @@ -779,9 +838,11 @@ xcb_generic_event_t *xcb_poll_for_special_event(xcb_connection_t *c, { xcb_generic_event_t *event; - if(c->has_error) - return 0; pthread_mutex_lock(&c->iolock); + if(c->has_error) { + pthread_mutex_unlock(&c->iolock); + return 0; + } event = get_special_event(c, se); if(!event && c->in.reading == 0 && _xcb_in_read(c)) /* _xcb_in_read shuts down the connection on error */ event = get_special_event(c, se); @@ -795,9 +856,11 @@ xcb_generic_event_t *xcb_wait_for_special_event(xcb_connection_t *c, special_list special; xcb_generic_event_t *event; - if(c->has_error) - return 0; pthread_mutex_lock(&c->iolock); + if(c->has_error) { + pthread_mutex_unlock(&c->iolock); + return 0; + } insert_special(&c->in.special_waiters, &special, se); @@ -822,12 +885,14 @@ xcb_register_for_special_xge(xcb_connection_t *c, xcb_special_event_t *se; const xcb_query_extension_reply_t *ext_reply; - if(c->has_error) - return NULL; ext_reply = xcb_get_extension_data(c, ext); if (!ext_reply) return NULL; pthread_mutex_lock(&c->iolock); + if (c->has_error) { + pthread_mutex_unlock(&c->iolock); + return NULL; + } for (se = c->in.special_events; se; se = se->next) { if (se->extension == ext_reply->major_opcode && se->eid == eid) { @@ -866,10 +931,11 @@ xcb_unregister_for_special_event(xcb_connection_t *c, if (!se) return; - if (c->has_error) - return; - pthread_mutex_lock(&c->iolock); + if (c->has_error) { + pthread_mutex_unlock(&c->iolock); + return NULL; + } for (prev = &c->in.special_events; (s = *prev) != NULL; prev = &(s->next)) { if (s == se) { diff --git a/src/xcbext.h b/src/xcbext.h index 90f9d58..d5ab36d 100644 --- a/src/xcbext.h +++ b/src/xcbext.h @@ -207,7 +207,7 @@ void xcb_send_fd(xcb_connection_t *c, int fd); * All replies that are generated while the socket is owned externally have * @p flags applied to them. For example, use XCB_REQUEST_CHECK if you don't * want errors to go to xcb's normal error handling, but instead having to be - * picked up via xcb_wait_for_reply(), xcb_poll_for_reply() or + * picked up via xcb_wait_for_reply_safe(), xcb_poll_for_reply_safe() or * xcb_request_check(). */ int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), void *closure, int flags, uint64_t *sent); @@ -236,11 +236,27 @@ int xcb_writev(xcb_connection_t *c, struct iovec *vector, int count, uint64_t re /* xcb_in.c */ +/** + * @brief Wait for the reply of a given request, with bounds checking. + * @param c The connection to the X server. + * @param request Sequence number of the request as returned by xcb_send_request(). + * @param e Location to store errors in, or NULL. Ignored for unchecked requests. + * @param min_size The minimum size of the reply, in bytes. + * + * Returns the reply to the given request or returns null in the event of + * errors. Blocks until the reply or error for the request arrives, or an I/O + * error occurs. The returned pointer is guaranteed to point to at least min_size + * bytes of data. + */ +void *xcb_wait_for_reply_safe(xcb_connection_t *c, unsigned int request, + xcb_generic_error_t **e, size_t min_size); + /** * @brief Wait for the reply of a given request. * @param c The connection to the X server. * @param request Sequence number of the request as returned by xcb_send_request(). * @param e Location to store errors in, or NULL. Ignored for unchecked requests. + * @deprecated Does not perform bounds checks; use xcb_wait_for_reply_safe() instead. * * Returns the reply to the given request or returns null in the event of * errors. Blocks until the reply or error for the request arrives, or an I/O @@ -253,6 +269,28 @@ void *xcb_wait_for_reply(xcb_connection_t *c, unsigned int request, xcb_generic_ * @param c The connection to the X server. * @param request 64-bit sequence number of the request as returned by xcb_send_request64(). * @param e Location to store errors in, or NULL. Ignored for unchecked requests. + * @param size The number of bytes expected in the reply. If the reply is too short, + * the connection will be shut down. If the return value is not null, it is guaranteed to + * point to size bytes of memory. + * + * Returns the reply to the given request or returns null in the event of + * errors. Blocks until the reply or error for the request arrives, or an I/O + * error occurs. + * + * Unlike its xcb_wait_for_reply_safe() counterpart, the given sequence number is not + * automatically "widened" to 64-bit. + */ +void *xcb_wait_for_reply64_safe(xcb_connection_t *c, uint64_t request, + xcb_generic_error_t **e, size_t size); + +/** + * @brief Wait for the reply of a given request, with 64-bit sequence number + * @param c The connection to the X server. + * @param request 64-bit sequence number of the request as returned by xcb_send_request64(). + * @param e Location to store errors in, or NULL. Ignored for unchecked requests. + * @deprecated This function does not perform bounds checks. If reply is not NULL on + * return, it is only guaranteed to point to 32 bytes of memory. Use + * xcb_wait_for_reply64_safe() instead. * * Returns the reply to the given request or returns null in the event of * errors. Blocks until the reply or error for the request arrives, or an I/O @@ -269,10 +307,47 @@ void *xcb_wait_for_reply64(xcb_connection_t *c, uint64_t request, xcb_generic_er * @param request Sequence number of the request as returned by xcb_send_request(). * @param reply Location to store the reply in, must not be NULL. * @param error Location to store errors in, or NULL. Ignored for unchecked requests. + * @param size The number of bytes expected in the reply. If the reply is too short, + * the connection will be shut down. If reply points to a non-NULL pointer on return, + * that pointer is guaranteed to point to size bytes of memory, or 32 bytes if size is + * less than 32. * @return 1 when the reply to the request was returned, else 0. * * Checks if the reply to the given request already received. Does not block. */ +int xcb_poll_for_reply_safe(xcb_connection_t *c, unsigned int request, void **reply, xcb_generic_error_t **error, size_t size); + +/** + * @brief Poll for the reply of a given request, with 64-bit sequence number. + * @param c The connection to the X server. + * @param request 64-bit sequence number of the request as returned by xcb_send_request(). + * @param reply Location to store the reply in, must not be NULL. + * @param error Location to store errors in, or NULL. Ignored for unchecked requests. + * @param size The number of bytes expected in the reply. If the reply is too short, + * the connection will be shut down. If reply points to a non-NULL pointer on return, + * that pointer is guaranteed to point to size bytes of memory, or 32 bytes if size is + * less than 32. + * @return 1 when the reply to the request was returned, else 0. + * + * Checks if the reply to the given request already received. Does not block. + * + * Unlike its xcb_poll_for_reply_safe() counterpart, the given sequence number is not + * automatically "widened" to 64-bit. + */ +int xcb_poll_for_reply64_safe(xcb_connection_t *c, uint64_t request, void **reply, xcb_generic_error_t **error, size_t size); + +/** + * @brief Poll for the reply of a given request. + * @param c The connection to the X server. + * @param request Sequence number of the request as returned by xcb_send_request(). + * @param reply Location to store the reply in, must not be NULL. + * @param error Location to store errors in, or NULL. Ignored for unchecked requests. + * @return 1 when the reply to the request was returned, else 0. + * @deprecated This function does not perform bounds checks. If reply is not NULL on + * return, it is only guaranteed to point to 32 bytes of memory. + * + * Checks if the reply to the given request already received. Does not block. + */ int xcb_poll_for_reply(xcb_connection_t *c, unsigned int request, void **reply, xcb_generic_error_t **error); /** @@ -282,6 +357,8 @@ int xcb_poll_for_reply(xcb_connection_t *c, unsigned int request, void **reply, * @param reply Location to store the reply in, must not be NULL. * @param error Location to store errors in, or NULL. Ignored for unchecked requests. * @return 1 when the reply to the request was returned, else 0. + * @deprecated This function does not perform bounds checks. If reply is not NULL on + * return, it is only guaranteed to point to 32 bytes of memory. * * Checks if the reply to the given request already received. Does not block. *