Re: Missing swapl() in Xext/saver.c?

[Keith Packard]

Mihai Moldovan  writes:

> Hi
>
>
> In
> https://cgit.freedesktop.org/xorg/xserver/commit/?id=9edcae78c46286baff42e74bfe26f6ae4d00fe01
> the swapl() call for stuff->suspend was removed. That sounds like a bad idea 
> to me?
>
> xScreenSaverSuspendReq to this day defines it as a Bool, which is typedef'd to
> int everywhere and includes the (ignored) B32 width specifier, so using 
> swapl()
> on the member sounds like a good idea. Definitely isn't a CARD8 object, 
> despite
> its name.

Oh. This is pretty bad. 'Bool' is not a valid type in protocol headers,
it should have been BOOL. And, BOOL is a single byte. But, it is Bool,
and I guess that is generally 4 bytes on most architectures.

The XCB headers define this field as a byte and have three pad bytes
afterwards, so we actually have incompatible protocol definitions for
xcb and xlib/xserver.

I'm not entirely sure what we should do at this point; I suspect that
treating it as a 32-bit value, fixing the protocol to use CARD32
everywhere instead of Bool and BOOL, and then adding swapl would cause
the least fuss.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Missing swapl() in Xext/saver.c?

[Mihai Moldovan]

* On 03/12/2018 05:13 AM, Keith Packard wrote:
> Oh. This is pretty bad. 'Bool' is not a valid type in protocol headers,
> it should have been BOOL. And, BOOL is a single byte. But, it is Bool,
> and I guess that is generally 4 bytes on most architectures.
>
> The XCB headers define this field as a byte and have three pad bytes
> afterwards, so we actually have incompatible protocol definitions for
> xcb and xlib/xserver.

I guess I'm about 7 years late to the party, but what has been done has been 
done.

Yep, xcb-proto files confirm this.

On the bright side, this likely hasn't caused actual issues on most platforms
with CPUs that use LE. CPUs in BE mode are quite rare.


> I'm not entirely sure what we should do at this point; I suspect that
> treating it as a 32-bit value, fixing the protocol to use CARD32
> everywhere instead of Bool and BOOL, and then adding swapl would cause
> the least fuss.

Sounds sane. Reverting to the initially intended BOOL type would cause breakage,
but using CARD32 + swapl() would leave the struct size consistent and fix the
issue in newer versions.

Not particularly clean, but least invasive.

A second option might be to create another request type with the initially
intended interface and stub out the original one, returning BadRequest. Sounds
like overkill, though...



Mihai



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel