[xserver] Xext/saver: Swap ScreenSaverSuspend 'suspend' field. Handle old XCB clients.

Submitted by Keith Packard on March 12, 2018, 7:27 p.m.

Details

Message ID 20180312192715.23904-1-keithp@keithp.com
State New
Series "Xext/saver: Swap ScreenSaverSuspend 'suspend' field. Handle old XCB clients."
Headers show

Commit Message

Keith Packard March 12, 2018, 7:27 p.m.
This field was defined as a Bool in the protocol headers and BOOL in
xcb. Bool is not a valid type for protocol fields. It is defined as
'int' by Xdefs.h, which we expect to be 32-bits on all machines.

The protocol headers and xcb have patches posted to switch to CARD32,
which is at least well defined.

This change adds the necessary byte swapping to handle other-endian
clients with this 32-bit field, and then changes the request
processing to use only the low byte of that value so that older XCB
clients will continue to work properly, at least on LSB machines.

On MSB machines, Xlib will continue to work properly, but old XCB will
not interoperate with the X server (either before or after this patch).

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 Xext/saver.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/Xext/saver.c b/Xext/saver.c
index 4d9e6b974..be7b403bb 100644
--- a/Xext/saver.c
+++ b/Xext/saver.c
@@ -1209,17 +1209,32 @@  static int
 ProcScreenSaverSuspend(ClientPtr client)
 {
     ScreenSaverSuspensionPtr *prev, this;
+    BOOL suspend;
 
     REQUEST(xScreenSaverSuspendReq);
     REQUEST_SIZE_MATCH(xScreenSaverSuspendReq);
 
+    /*
+     * Old versions of XCB encode suspend as 1 byte followed by three
+     * pad bytes, instead of a 4 byte value.  Be compatible on LSB
+     * machines by only using the low byte. There's not much we can do
+     * for MSB machines here as the true value will be in the high
+     * byte for old XCB and the low byte for new XCB, but always in the
+     * low byte for Xlib.
+     */
+    suspend = stuff->suspend & 0xff;
+    if (suspend != TRUE && suspend != FALSE) {
+        client->errorValue = stuff->suspend;
+        return BadValue;
+    }
+
     /* Check if this client is suspending the screensaver */
     for (prev = &suspendingClients; (this = *prev); prev = &this->next)
         if (this->pClient == client)
             break;
 
     if (this) {
-        if (stuff->suspend == TRUE)
+        if (suspend == TRUE)
             this->count++;
         else if (--this->count == 0)
             FreeResource(this->clientResource, RT_NONE);
@@ -1228,7 +1243,7 @@  ProcScreenSaverSuspend(ClientPtr client)
     }
 
     /* If we get to this point, this client isn't suspending the screensaver */
-    if (stuff->suspend == FALSE)
+    if (suspend == FALSE)
         return Success;
 
     /*
@@ -1342,6 +1357,7 @@  SProcScreenSaverSuspend(ClientPtr client)
     REQUEST(xScreenSaverSuspendReq);
 
     swaps(&stuff->length);
+    swapl(&stuff->suspend);
     REQUEST_SIZE_MATCH(xScreenSaverSuspendReq);
     return ProcScreenSaverSuspend(client);
 }

Comments

Mihai Moldovan March 13, 2018, 8:48 a.m.
* On 03/12/2018 08:27 PM, Keith Packard wrote:
> This field was defined as a Bool in the protocol headers and BOOL in
> xcb. Bool is not a valid type for protocol fields. It is defined as
> 'int' by Xdefs.h, which we expect to be 32-bits on all machines.
> 
> The protocol headers and xcb have patches posted to switch to CARD32,
> which is at least well defined.
> 
> This change adds the necessary byte swapping to handle other-endian
> clients with this 32-bit field, and then changes the request
> processing to use only the low byte of that value so that older XCB
> clients will continue to work properly, at least on LSB machines.
> 
> On MSB machines, Xlib will continue to work properly, but old XCB will
> not interoperate with the X server (either before or after this patch).

An actual matrix for this would be

C -> S |       old server         |       new server
       | Xlib | old xcb | new xcb | Xlib | old xcb | new xcb
------------------------------------------------------------
l -> l |   X  |    X    |    X    |   X  |    X    |    X
l -> B |   -  |    -    |    -    |   X  |    X    |    X
B -> l |   -  |    X    |    -    |   X  |    -    |    X
B -> B |   X  |    -    |    X    |   X  |    -    |    X


Too complicated to fully transcribe into words as part of a commit message, I'd say.


The actual changes (both server and proto) LGTM.



Mihai
Keith Packard March 14, 2018, 4:19 a.m.
Mihai Moldovan <ionic@ionic.de> writes:

> The actual changes (both server and proto) LGTM.

I'll take that as a 'Reviewed-by', unless you have some objection.
Mihai Moldovan March 14, 2018, 9:30 a.m.
* On 03/14/2018 05:19 AM, Keith Packard wrote:
> I'll take that as a 'Reviewed-by', unless you have some objection.

Sure, go ahead!