reading/writing xproto.SetupRequest is missing padding

[pphaneuf]

The AuthorizationProtocolName and AuthorizationProtocolData fields are supposed to be padded to a multiple of 4, but it's not done:

https://github.com/BurntSushi/xgb/blob/eabb7feb995f0d542f1972909a69f4ab02c8f210/xproto/xproto.go#L5805

https://github.com/BurntSushi/xgb/blob/eabb7feb995f0d542f1972909a69f4ab02c8f210/xproto/xproto.go#L5853

I see some mentions of xgb.Pad() at line 5830, but it comes out a bit short when actually using it?

[aarzilli]

How would one know that AuthorizationProtocolName and AuthorizationProtocolData are to be padded, short of reading xlib code? Issue #12 was caused by strings being padded when they shouldn't have been.

Note that conn.go doesn't use SetupRequest to setup the connection (and that does pad the fields).

[pphaneuf]

I did this by trying to write my own version of the connect() function that uses xproto.SetupRequestRead and xproto.SetupRequest.Bytes(), and got it wrong (server rejected the connection). I did some debugging by dumping buffers to stdout, and reading through them, as well as tracing what a normal client (I used xdpyinfo) was sending, and found some extra bytes.

I then looked at the connect() function itself, and found this:

https://github.com/BurntSushi/xgb/blob/master/conn.go#L57

It doesn't use the xproto.SetupRequest stuff (as you mention), but instead re-implement it "by hand", and there was the missing padding!

[pphaneuf]

It'd be kind of nice if connect() used xproto.SetupRequest.Bytes(), no? It shouldn't be hard either (other than this bug!), would just replace lines 48 to 57 of connect() by populating a xproto.SetupRequest struct, and using xproto.SetupRequest.Bytes() at line 58...

[BurntSushi]

It's not that simple unfortunately. The xgb/xproto package depends on xgb. Copy and paste to fix it should be fine. It's not going to change.

[pphaneuf]

Oh, I see... Many of those look like they're helper functions, but not all, getting the last reference out might be tricky.

I'd still like to use xproto.SetupRequestRead and xproto.SetupRequest.Bytes(), though, it'd be nice if they worked. :wink:

[BurntSushi]

https://github.com/BurntSushi/xgb/blob/master/sync.go#L14 :-)

[pphaneuf]

As in, "we already copy/paste functions wholesale, it's all right"? Sure, but if you copy/pasted xproto.SetupRequest.Bytes() and used it in connect(), you'd break it. :smiley:

If it wasn't going through the compiler, I'd have just sent a pull request, but I'm not as comfortable making that change to the compiler, sorry...

[aarzilli]

If it wasn't going through the compiler, I'd have just sent a pull request, but I'm not as comfortable making that change to the compiler, sorry...

Yeah, that's exactly the problem: X11 padding rules aren't really clear (or documented at all). People will tell you that "it's just like C struct padding", except it isn't, not in any interesting place, at least. I look at that SetupRequest definition in the XML and I see no reason why there would be padding.

[pphaneuf]

Might be a bug in the XML, then? Here, it's clearly mentioned:

http://www.x.org/releases/current/doc/xproto/x11protocol.html#Encoding::Connection_Setup

I hear you on the padding rules not being the clearest!

[pphaneuf]

(I'm referring to the "p" and "q" of the very first structure description there in the "Connection Setup" section)

[aarzilli]

You may be onto something, I ran a capture myself and xcb proper seems to have similar problems. I'll inquire upstream.