Bug 49166

(In reply to comment #1)
> [...] the issue can be found in src/glx/indirect_util.c where the
> __glXSendReplySwap() method does a "bswap16( client->sequence )" (line 176 in
> current X.org git) while it should do "bswap16( client->sequence & 0xFFFF )".

I'd argue that shouldn't be necessary, or there's a bug in bswap_16(). With current xserver Git, which implementation does glx/glxbyteorder.h end up using for you? I can definitely see how the fallback implementation:

#define bswap_16(value)  \
        ((((value) & 0xff) << 8) | ((value) >> 8))

could cause this problem, as it doesn't mask the value shifted down.


P.S. The reporter normally receives all bug mail anyway, you shouldn't need to be in the CC list.

(In reply to comment #1)
> I can definitely see how the fallback implementation:
> 
> #define bswap_16(value)  \
>         ((((value) & 0xff) << 8) | ((value) >> 8))
> 
> could cause this problem, as it doesn't mask the value shifted down.

Indeed. I hesitated between to two fixes options in my report, but as for me it concerned specifically the sequence numbers (32 bits integer internally, but 16 bit short in the protocol), I chose the other one :)

> 
> P.S. The reporter normally receives all bug mail anyway, you shouldn't need to
> be in the CC list.

Good to know. First time using a bugzilla tracker in almost 7 years. Thanks.

(In reply to comment #1)
> 
> I'd argue that shouldn't be necessary, or there's a bug in bswap_16(). With
> current xserver Git, which implementation does glx/glxbyteorder.h end up using
> for you? 

I'd settle for the fallback one, but cannot guarantee it 100%.

Link for the ATI driver bug showing same behavior : http://ati.cchtml.com/show_bug.cgi?id=490

Still waiting on feedback from originator.

What additional feedback do you expect ?
Everything was described in the first post. I can only add that it definitely uses the "fallback" method cited earlier.

Best regards,

Your response in comment 3 doesn't really answer the question, and it's confusing.  From your comment 6, I take it to mean that in your case it is using the macro that Michel pointed you to.

Have you tried the fix that he suggested?

Indeed, comment 6 meant that it is using that macro.

What fix are you referring to ?

(In reply to comment #8)
> Indeed, comment 6 meant that it is using that macro.
> 
> What fix are you referring to ?

From comment 1, "mask the value shifted down"

ie, change it to:

#define bswap_16(value)  \
        ((((value) & 0xff) << 8) | ((value & 0xff00) >> 8))

(In reply to comment #10)
> ie, change it to:
> 
> #define bswap_16(value)  \
>         ((((value) & 0xff) << 8) | ((value & 0xff00) >> 8))

or rather:

#define bswap_16(value)  \
        ((((value) & 0xff) << 8) | (((value) & 0xff00) >> 8))

Our current setup does not allow us to recompile X.org with this modification. Moreover, this lib is overwritten by the binary drivers that we use, that contain the same issue. We worked around this by using an X11 proxy that corrects "on-the-fly" those sequence numbers.

Hi folks,
  Has any progress been made on this?

Fixed (somewhat accidentally) by:

commit 563b6ee873b898c0f3e3671cf6adaf91def5d92a
Author: Eric Anholt <eric@anholt.net>
Date:   Mon Mar 27 14:59:06 2017 -0700

    Rewrite the byte swapping macros.