From 3a113815a0cc86d64789494e905da9778174f738 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Mon, 6 Jan 2014 17:10:38 -0800 Subject: [PATCH 1/6] If AllocGrab() fails to set up grab, don't copy to a NULL grab If either the initial calloc or the xi2mask_new fails, grab is NULL, but if a src grab is passed in, it was always being written to by CopyGrab (and if that failed, dereferenced again in teardown). Signed-off-by: Alan Coopersmith Signed-off-by: Peter Hutterer Reviewed-by: Peter Hutterer --- dix/grabs.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/dix/grabs.c b/dix/grabs.c index a03897af4..7f4c8715e 100644 --- a/dix/grabs.c +++ b/dix/grabs.c @@ -199,12 +199,11 @@ AllocGrab(const GrabPtr src) free(grab); grab = NULL; } - } - - if (src && !CopyGrab(grab, src)) { - free(grab->xi2mask); - free(grab); - grab = NULL; + else if (src && !CopyGrab(grab, src)) { + free(grab->xi2mask); + free(grab); + grab = NULL; + } } return grab; From 5493a67ec256d22a8a41597a345d8e1c54d6e335 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Mon, 6 Jan 2014 17:10:39 -0800 Subject: [PATCH 2/6] GrabDevice() needs to handle NULL return value from AllocGrab() GrabDevice() calls AllocGrab() which can fail and return NULL. This return value is not checked, and can cause NULL pointer dereferences. Reported-by: Ilja Van Sprundel Signed-off-by: Alan Coopersmith Signed-off-by: Peter Hutterer Reviewed-by: Peter Hutterer --- dix/events.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dix/events.c b/dix/events.c index 4aaa54c85..2f0605ef5 100644 --- a/dix/events.c +++ b/dix/events.c @@ -5051,7 +5051,7 @@ ProcUngrabPointer(ClientPtr client) * @param other_mode GrabModeSync or GrabModeAsync * @param status Return code to be returned to the caller. * - * @returns Success or BadValue. + * @returns Success or BadValue or BadAlloc. */ int GrabDevice(ClientPtr client, DeviceIntPtr dev, @@ -5132,6 +5132,8 @@ GrabDevice(ClientPtr client, DeviceIntPtr dev, GrabPtr tempGrab; tempGrab = AllocGrab(NULL); + if (tempGrab == NULL) + return BadAlloc; tempGrab->next = NULL; tempGrab->window = pWin; From 863d2ad5c02cccde9a4d1a392a7cae78d001c8a9 Mon Sep 17 00:00:00 2001 From: Alan Coopersmith Date: Mon, 6 Jan 2014 17:10:40 -0800 Subject: [PATCH 3/6] CheckPassiveGrabsOnWindow() needs to handle NULL return value from AllocGrab() CheckPassiveGrabsOnWindow() calls AllocGrab() which can fail and return NULL. This return value is not checked, and can cause NULL pointer dereferences. Signed-off-by: Alan Coopersmith Signed-off-by: Peter Hutterer Reviewed-by: Peter Hutterer --- dix/events.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dix/events.c b/dix/events.c index 2f0605ef5..acf97cc10 100644 --- a/dix/events.c +++ b/dix/events.c @@ -3956,6 +3956,8 @@ CheckPassiveGrabsOnWindow(WindowPtr pWin, return NULL; tempGrab = AllocGrab(NULL); + if (tempGrab == NULL) + return NULL; /* Fill out the grab details, but leave the type for later before * comparing */ From b2d5ee2e3684951b611fd2068d57cc65fd8305a3 Mon Sep 17 00:00:00 2001 From: Carlos Garnacho Date: Thu, 2 Jan 2014 21:33:30 +0100 Subject: [PATCH 4/6] Xi: Ensure DeviceChanged is emitted after grabs are deactivated When a grab on a slave device is deactivated, the master device must be checked, just in case there were events from other devices while the slave device was stolen away by the passive grab. This may introduce misbehaviors on mismatching valuators and device features later on UpdateDeviceState(). Signed-off-by: Carlos Garnacho Reviewed-by: Peter Hutterer Signed-off-by: Peter Hutterer --- Xi/exevents.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/Xi/exevents.c b/Xi/exevents.c index dff0a92b0..528e105a3 100644 --- a/Xi/exevents.c +++ b/Xi/exevents.c @@ -1783,8 +1783,25 @@ ProcessDeviceEvent(InternalEvent *ev, DeviceIntPtr device) DeliverDeviceEvents(GetSpriteWindow(device), (InternalEvent *) event, NullGrab, NullWindow, device); - if (deactivateDeviceGrab == TRUE) + if (deactivateDeviceGrab == TRUE) { (*device->deviceGrab.DeactivateGrab) (device); + + if (!IsMaster (device) && !IsFloating (device)) { + int flags, num_events = 0; + InternalEvent dce; + + flags = (IsPointerDevice (device)) ? + DEVCHANGE_POINTER_EVENT : DEVCHANGE_KEYBOARD_EVENT; + UpdateFromMaster (&dce, device, flags, &num_events); + BUG_WARN(num_events > 1); + + if (num_events == 1) + ChangeMasterDeviceClasses(GetMaster (device, MASTER_ATTACHED), + &dce.changed_event); + } + + } + event->detail.key = key; } From e81902872176fa9848211fcd7a5eafa4f861a1b7 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Thu, 9 Jan 2014 15:29:23 +1000 Subject: [PATCH 5/6] ephyr: don't allow a shift+ctrl keygrab if mod1 was enabled Xephyr wants ctrl+shift to grab the window, but that conflicts with ctrl+alt+shift key combos. Remember the modifier state on key presses and releases, if mod1 is pressed, we need ctrl, shift and mod1 released before we allow a shift-ctrl grab activation. Signed-off-by: Peter Hutterer --- hw/kdrive/ephyr/ephyr.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/hw/kdrive/ephyr/ephyr.c b/hw/kdrive/ephyr/ephyr.c index b2a79855c..94b93970e 100644 --- a/hw/kdrive/ephyr/ephyr.c +++ b/hw/kdrive/ephyr/ephyr.c @@ -1008,6 +1008,29 @@ ephyrProcessButtonRelease(xcb_generic_event_t *xev) KdEnqueuePointerEvent(ephyrMouse, mouseState | KD_MOUSE_DELTA, 0, 0, 0); } +/* Xephyr wants ctrl+shift to grab the window, but that conflicts with + ctrl+alt+shift key combos. Remember the modifier state on key presses and + releases, if mod1 is pressed, we need ctrl, shift and mod1 released + before we allow a shift-ctrl grab activation. + + note: a key event contains the mask _before_ the current key takes + effect, so mod1_was_down will be reset on the first key press after all + three were released, not on the last release. That'd require some more + effort. + */ +static int +ephyrUpdateGrabModifierState(int state) +{ + static int mod1_was_down = 0; + + if ((state & (XCB_MOD_MASK_CONTROL|XCB_MOD_MASK_SHIFT|XCB_MOD_MASK_1)) == 0) + mod1_was_down = 0; + else if (state & XCB_MOD_MASK_1) + mod1_was_down = 1; + + return mod1_was_down; +} + static void ephyrProcessKeyPress(xcb_generic_event_t *xev) { @@ -1018,6 +1041,7 @@ ephyrProcessKeyPress(xcb_generic_event_t *xev) return; } + ephyrUpdateGrabModifierState(key->state); ephyrUpdateModifierState(key->state); KdEnqueueKeyboardEvent(ephyrKbd, key->detail, FALSE); } @@ -1029,6 +1053,7 @@ ephyrProcessKeyRelease(xcb_generic_event_t *xev) xcb_key_release_event_t *key = (xcb_key_release_event_t *)xev; static xcb_key_symbols_t *keysyms; static int grabbed_screen = -1; + int mod1_down = ephyrUpdateGrabModifierState(key->state); if (!keysyms) keysyms = xcb_key_symbols_alloc(conn); @@ -1049,7 +1074,7 @@ ephyrProcessKeyRelease(xcb_generic_event_t *xev) hostx_set_win_title(screen, "(ctrl+shift grabs mouse and keyboard)"); } - else { + else if (!mod1_down) { /* Attempt grab */ xcb_grab_keyboard_cookie_t kbgrabc = xcb_grab_keyboard(conn, From 71baa466b1f6b02fe503f9a3089b7b9d61aa0f80 Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Mon, 13 Jan 2014 17:00:23 +1000 Subject: [PATCH 6/6] os: restrict display names to digits We call atoi() on the server's display to get the socket but otherwise use the unmodified display for log file name, xkb paths, etc. This results in Xorg :banana being the equivalent of Xorg :0, except for the log files being in /var/log/Xorg.banana.log. I'm not sure there's a good use-case for this behaviour. Check the display for something that looks reasonable, i.e. digits only, but do allow for :0.0 (i.e. digits, followed by a period, followed by one or two digits). Signed-off-by: Peter Hutterer Reviewed-by: Keith Packard --- os/utils.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/os/utils.c b/os/utils.c index 608ee6ab0..3b20a5ccb 100644 --- a/os/utils.c +++ b/os/utils.c @@ -600,6 +600,10 @@ UseMsg(void) static int VerifyDisplayName(const char *d) { + int i; + int period_found = FALSE; + int after_period = 0; + if (d == (char *) 0) return 0; /* null */ if (*d == '\0') @@ -610,6 +614,29 @@ VerifyDisplayName(const char *d) return 0; /* must not equal "." or ".." */ if (strchr(d, '/') != (char *) 0) return 0; /* very important!!! */ + + /* Since we run atoi() on the display later, only allow + for digits, or exception of :0.0 and similar (two decimal points max) + */ + for (i = 0; i < strlen(d); i++) { + if (!isdigit(d[i])) { + if (d[i] != '.' || period_found) + return 0; + period_found = TRUE; + } else if (period_found) + after_period++; + + if (after_period > 2) + return 0; + } + + /* don't allow for :0. */ + if (period_found && after_period == 0) + return 0; + + if (atol(d) > INT_MAX) + return 0; + return 1; }