File U_0006-Fix-poll_for_response-race-condition.patch of Package libX11.29367

Overview Repositories Revisions Requests Users Attributes Meta

File U_0006-Fix-poll_for_response-race-condition.patch of Package libX11.29367

From dbb55e1a5e82870466b095097d9e46046680ec25 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@redhat.com>
Date: Wed, 29 Jan 2020 09:06:54 +0000
Subject: [PATCH] Fix poll_for_response race condition

In poll_for_response is it possible that event replies are skipped
and a more up to date message reply is returned.
This will cause next poll_for_event call to fail aborting the program.

This was proved using some slow ssh tunnel or using some program
to slow down server replies (I used a combination of xtrace and strace).

How the race happens:
- program enters into poll_for_response;
- poll_for_event is called but the server didn't still send the reply;
- pending_requests is not NULL because we send a request (see call
  to  append_pending_request in _XSend);
- xcb_poll_for_reply64 is called from poll_for_response;
- xcb_poll_for_reply64 will read from server, at this point
  server reply with an event (say sequence N) and the reply to our
  last request (say sequence N+1);
- xcb_poll_for_reply64 returns the reply for the request we asked;
- last_request_read is set to N+1 sequence in poll_for_response;
- poll_for_response returns the response to the request;
- poll_for_event is called (for instance from another poll_for_response);
- event with sequence N is retrieved;
- the N sequence is widen, however, as the "new" number computed from
  last_request_read is less than N the number is widened to N + 2^32
  (assuming last_request_read is still contained in 32 bit);
- poll_for_event enters the nested if statement as req is NULL;
- we compare the widen N (which now does not fit into 32 bit) with
  request (which fits into 32 bit) hitting the throw_thread_fail_assert.

To avoid the race condition and to avoid the sequence to go back
I check again for new events after getting the response and
return this last event if present saving the reply to return it
later.

To test the race and the fix it's helpful to add a delay (I used a
"usleep(5000)") before calling xcb_poll_for_reply64.

Original patch written by Frediano Ziglio, see
https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/34

Reworked primarily for readability by Peter Hutterer, see
https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/53

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
 src/Xxcbint.h |  1 +
 src/xcb_io.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 71 insertions(+), 9 deletions(-)

diff --git a/src/Xxcbint.h b/src/Xxcbint.h
index 4ef13d2f..d8293259 100644
--- a/src/Xxcbint.h
+++ b/src/Xxcbint.h
@@ -27,6 +27,7 @@ typedef struct _X11XCBPrivate {
 	PendingRequest *pending_requests;
 	PendingRequest *pending_requests_tail;
 	xcb_generic_event_t *next_event;
+	void *next_response;
 	char *real_bufmax;
 	char *reply_data;
 	int reply_length;
diff --git a/src/xcb_io.c b/src/xcb_io.c
index 0bd1fdf9..4be75408 100644
--- a/src/xcb_io.c
+++ b/src/xcb_io.c
@@ -282,22 +282,83 @@ static xcb_generic_reply_t *poll_for_event(Display *dpy, Bool queued_only)
 static xcb_generic_reply_t *poll_for_response(Display *dpy)
 {
 	void *response;
-	xcb_generic_error_t *error;
+	xcb_generic_reply_t *event;
 	PendingRequest *req;
-	while(!(response = poll_for_event(dpy, False)) &&
-	      (req = dpy->xcb->pending_requests) &&
-	      !req->reply_waiter)
+
+	while(1)
 	{
+		xcb_generic_error_t *error = NULL;
 		uint64_t request;
+		Bool poll_queued_only = dpy->xcb->next_response != NULL;
+
+		/* Step 1: is there an event in our queue before the next
+		 * reply/error? Return that first.
+		 *
+		 * If we don't have a reply/error saved from an earlier
+		 * invocation we check incoming events too, otherwise only
+		 * the ones already queued.
+		 */
+		response = poll_for_event(dpy, poll_queued_only);
+		if(response)
+			break;
 
-		if(!xcb_poll_for_reply64(dpy->xcb->connection, req->sequence,
-					 &response, &error)) {
-			/* xcb_poll_for_reply64 may have read events even if
-			 * there is no reply. */
-			response = poll_for_event(dpy, True);
+		/* Step 2:
+		 * Response is NULL, i.e. we have no events.
+		 * If we are not waiting for a reply or some other thread
+		 * had dibs on the next reply, exit.
+		 */
+		req = dpy->xcb->pending_requests;
+		if(!req || req->reply_waiter)
 			break;
+
+		/* Step 3:
+		 * We have some response (error or reply) related to req
+		 * saved from an earlier invocation of this function. Let's
+		 * use that one.
+		 */
+		if(dpy->xcb->next_response)
+		{
+			if (((xcb_generic_reply_t*)dpy->xcb->next_response)->response_type == X_Error)
+			{
+				error = dpy->xcb->next_response;
+				response = NULL;
+			}
+			else
+			{
+				response = dpy->xcb->next_response;
+				error = NULL;
+			}
+			dpy->xcb->next_response = NULL;
+		}
+		else
+		{
+			/* Step 4: pull down the next response from the wire. This
+			 * should be the 99% case.
+			 * xcb_poll_for_reply64() may also pull down events that
+			 * happened before the reply.
+			 */
+			if(!xcb_poll_for_reply64(dpy->xcb->connection, req->sequence,
+						 &response, &error)) {
+				/* if there is no reply/error, xcb_poll_for_reply64
+				 * may have read events. Return that. */
+				response = poll_for_event(dpy, True);
+				break;
+			}
+
+			/* Step 5: we have a new response, but we may also have some
+			 * events that happened before that response. Return those
+			 * first and save our reply/error for the next invocation.
+			 */
+			event = poll_for_event(dpy, True);
+			if(event)
+			{
+				dpy->xcb->next_response = error ? error : response;
+				response = event;
+				break;
+			}
 		}
 
+		/* Step 6: actually handle the reply/error now... */
 		request = X_DPY_GET_REQUEST(dpy);
 		if(XLIB_SEQUENCE_COMPARE(req->sequence, >, request))
 		{
-- 
2.16.4

Places

File U_0006-Fix-poll_for_response-race-condition.patch of Package libX11.29367

Places