Is SendRequest missing some padding?

Discussion:

Alessandro Arzilli

2014-09-18 12:29:03 UTC

Hello,
X.org X11 protocol documentation specifies that the fields
AuthorizationProtocolName and AuthorizationProtocolData should be padded
(http://www.x.org/releases/current/doc/xproto/x11protocol.html#Encoding::Connection_Setup),
xproto.xml however doesn't mention this padding anywhere.

For context, I found this out because of this:
https://github.com/BurntSushi/xgb/issues/24
I also ran a capture with wireshark and plugged the SetupRequest packet
in this program (https://gist.github.com/aarzilli/fcd2a0094f89c29256a9)
and it seems that xcb also isn't decoding the padding correctly (but
maybe I'm doing it wrong?).

Also, just adding <pad align="4" /> after the two fields didn't work for
me, I just got a compile error with libxcb-1.11.
This, much uglier, change did work:
---
--- xproto.xml.old 2014-09-18 13:34:17.794910423 +0200
+++ xproto.xml 2014-09-18 13:54:38.946273390 +0200
@@ -193,10 +193,28 @@
<field type="CARD16" name="authorization_protocol_data_len" />
<pad bytes="2" />
<list type="char" name="authorization_protocol_name">
- <fieldref>authorization_protocol_name_len</fieldref>
+ <op op="*">
+ <op op="/">
+ <op op="+">
+ <fieldref>authorization_protocol_name_len</fieldref>
+ <value>3</value>
+ </op>
+ <value>4</value>
+ </op>
+ <value>4</value>
+ </op>
</list>
<list type="char" name="authorization_protocol_data">
- <fieldref>authorization_protocol_data_len</fieldref>
+ <op op="*">
+ <op op="/">
+ <op op="+">
+ <fieldref>authorization_protocol_data_len</fieldref>
+ <value>3</value>
+ </op>
+ <value>4</value>
+ </op>
+ <value>4</value>
+ </op>
</list>
</struct>
---

Thank you,
Alessandro Arzilli

chris-B8Tg/

2014-09-18 17:27:47 UTC

Permalink

Hello Alessandro,

I think we should fix the problem in the generator with <pad align="4" />.

Can you please try if <pad align="4" /> works with my newest patches which are not yet merged into upstream?

You can get them with
git clone http://infra-srv1.demorecorder.com/git/free-sw/xcb/libxcb/
git clone http://infra-srv1.demorecorder.com/git/free-sw/xcb/proto/

Regards,

Chris

Post by Alessandro Arzilli
Hello,
X.org X11 protocol documentation specifies that the fields
AuthorizationProtocolName and AuthorizationProtocolData should be padded
(http://www.x.org/releases/current/doc/xproto/x11protocol.html#Encoding::Connection_Setup),
xproto.xml however doesn't mention this padding anywhere.
https://github.com/BurntSushi/xgb/issues/24
I also ran a capture with wireshark and plugged the SetupRequest packet
in this program (https://gist.github.com/aarzilli/fcd2a0094f89c29256a9)
and it seems that xcb also isn't decoding the padding correctly (but
maybe I'm doing it wrong?).
Also, just adding <pad align="4" /> after the two fields didn't work for
me, I just got a compile error with libxcb-1.11.
---
--- xproto.xml.old 2014-09-18 13:34:17.794910423 +0200
+++ xproto.xml 2014-09-18 13:54:38.946273390 +0200
@@ -193,10 +193,28 @@
<field type="CARD16" name="authorization_protocol_data_len" />
<pad bytes="2" />
<list type="char" name="authorization_protocol_name">
- <fieldref>authorization_protocol_name_len</fieldref>
+ <op op="*">
+ <op op="/">
+ <op op="+">
+ <fieldref>authorization_protocol_name_len</fieldref>
+ <value>3</value>
+ </op>
+ <value>4</value>
+ </op>
+ <value>4</value>
+ </op>
</list>
<list type="char" name="authorization_protocol_data">
- <fieldref>authorization_protocol_data_len</fieldref>
+ <op op="*">
+ <op op="/">
+ <op op="+">
+ <fieldref>authorization_protocol_data_len</fieldref>
+ <value>3</value>
+ </op>
+ <value>4</value>
+ </op>
+ <value>4</value>
+ </op>
</list>
</struct>
---
Thank you,
Alessandro Arzilli
_______________________________________________
Xcb mailing list
http://lists.freedesktop.org/mailman/listinfo/xcb

Christian Linhart

2014-09-19 08:45:30 UTC

Permalink

Hi Alessandro,

Thank you for checking that. Yes, you have checked out the correct branch.

I'll look into this.
Maybe it's easy to fix.

Chris

Post by chris-B8Tg/
Hello Alessandro,
I think we should fix the problem in the generator with <pad align="4" />.
Can you please try if <pad align="4" /> works with my newest patches which are not yet merged into upstream?
You can get them with
git clone http://infra-srv1.demorecorder.com/git/free-sw/xcb/libxcb/
git clone http://infra-srv1.demorecorder.com/git/free-sw/xcb/proto/

I get the same error I got with libxcb-1.11.

[...]

Christian Linhart

2014-09-19 11:44:38 UTC

Permalink

Handle align-pads when generating an end-function
in the same way as handling them when generating
an accessor or iterator function.

This patch can be applied to branch master
or to the result of my pending patchsets.
---
src/c_client.py | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/c_client.py b/src/c_client.py
index 33c86b3..b5a1e0c 100644
--- a/src/c_client.py
+++ b/src/c_client.py
@@ -1976,18 +1976,24 @@ def _c_accessors_list(self, field):
if switch_obj is not None:
_c(' i.data = %s + %s;', fields[field.c_field_name][0],
_c_accessor_get_expr(field.type.expr, fields))
elif field.prev_varsized_field == None:
_c(' i.data = ((%s *) (R + 1)) + (%s);', field.type.c_wiretype,
_c_accessor_get_expr(field.type.expr, fields))
else:
- _c(' xcb_generic_iterator_t child = %s;',
- _c_iterator_get_end(field.prev_varsized_field, 'R'))
- _c(' i.data = ((%s *) child.data) + (%s);', field.type.c_wiretype,
- _c_accessor_get_expr(field.type.expr, fields))
+ (prev_varsized_field, align_pad) = get_align_pad(field)
+
+ if align_pad is None:
+ align_pad = ('XCB_TYPE_PAD(%s, prev.index)' %
+ type_pad_type(field.first_field_after_varsized.type.c_type))
+
+ _c(' xcb_generic_iterator_t prev = %s;',
+ _c_iterator_get_end(prev_varsized_field, 'R'))
+ _c(' i.data = ((%s *) prev.data) + %s + (%s);', field.type.c_wiretype,
+ align_pad, _c_accessor_get_expr(field.type.expr, fields))

_c(' i.rem = 0;')
_c(' i.index = (char *) i.data - (char *) %s;', param)
_c(' return i;')
_c('}')

else:

--
2.0.1

Christian Linhart

2015-08-13 16:15:29 UTC

Permalink

I have pushed this patch now.

It got more than enough time for review and nobody complained, so it's probably ok.

It's needed for properly reflect padding between lists in the end-iterator functions
( this applies to both explicit and implicit padding)

I need that fix as a prerequisite for another bugfix which I'll post (hopefully this weekend, but can take longer...).

Chris

Post by Christian Linhart
Handle align-pads when generating an end-function
in the same way as handling them when generating
an accessor or iterator function.
This patch can be applied to branch master
or to the result of my pending patchsets.
---
src/c_client.py | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/c_client.py b/src/c_client.py
index 33c86b3..b5a1e0c 100644
--- a/src/c_client.py
+++ b/src/c_client.py
_c(' i.data = %s + %s;', fields[field.c_field_name][0],
_c_accessor_get_expr(field.type.expr, fields))
_c(' i.data = ((%s *) (R + 1)) + (%s);', field.type.c_wiretype,
_c_accessor_get_expr(field.type.expr, fields))
- _c(' xcb_generic_iterator_t child = %s;',
- _c_iterator_get_end(field.prev_varsized_field, 'R'))
- _c(' i.data = ((%s *) child.data) + (%s);', field.type.c_wiretype,
- _c_accessor_get_expr(field.type.expr, fields))
+ (prev_varsized_field, align_pad) = get_align_pad(field)
+
+ align_pad = ('XCB_TYPE_PAD(%s, prev.index)' %
+ type_pad_type(field.first_field_after_varsized.type.c_type))
+
+ _c(' xcb_generic_iterator_t prev = %s;',
+ _c_iterator_get_end(prev_varsized_field, 'R'))
+ _c(' i.data = ((%s *) prev.data) + %s + (%s);', field.type.c_wiretype,
+ align_pad, _c_accessor_get_expr(field.type.expr, fields))
_c(' i.rem = 0;')
_c(' i.index = (char *) i.data - (char *) %s;', param)
_c(' return i;')
_c('}')

Alessandro Arzilli

2015-08-31 12:17:09 UTC

Permalink

Patch attached.

Alessandro Arzilli.

Post by Christian Linhart
Hi Alessandro,
You are welcome.
Yes, the missing align-pads will be added because I have an upcoming patch that automatically checks
for missing align-pads. Therefore, all missing align-pads will eventually get fixed.
However, to be sure, you can post a patch to the xcb-mailinglist.
(make sure that your patch applies cleanly with "git am".)
Chris

Thanks! Are the <pad align="4"/> tags going to get added to SetupRequest as well?

Post by Christian Linhart
Hi Alessandro,
http://cgit.freedesktop.org/xcb/libxcb/commit/?id=4033d39d4da21842bb1396a419dfc299591c3b1f
Sorry for the long time between posting my patch and pushing it.
(Back then I didn't have commit rights, yet. Later I forgot about that patch.)
Thanks again for reporting that problem back then.
Cheers,
Chris

Post by Christian Linhart
Hi Alessandro,
Thank you for checking that. Yes, you have checked out the correct branch.
I'll look into this.
Maybe it's easy to fix.
Chris

I get the same error I got with libxcb-1.11.

[...]
_______________________________________________
Xcb mailing list
http://lists.freedesktop.org/mailman/listinfo/xcb

Christian Linhart

2015-09-13 08:40:25 UTC

Permalink

Hi Alessandro,

Thank you for this patch.
The upcoming auto-align-check would not have found it because missing pads would not cause any primitive data-types larger than 8bits to be misaligned.

I have two requests for getting this patch to the official repo:
1. Can you please repost it in the usual format of posting patches:
I.e., generated by "git format-patch",
so that the subjectline of the mail is prefixed with [PATCH proto],
and so that the patch is included in the main message.

This helps the people on the mailinglist to easily recognize your post as a patch to be reviewed.

2. Can you please include an explanation,
and a link to the spec or source code which shows
that these fields really should be 4-byte aligned?

If you need help with any of these, please tell me.

Cheers,

Chris

Post by Alessandro Arzilli
Patch attached.
Alessandro Arzilli.

Thanks! Are the <pad align="4"/> tags going to get added to SetupRequest as well?

Post by Christian Linhart
Hi Alessandro,
Thank you for checking that. Yes, you have checked out the correct branch.
I'll look into this.
Maybe it's easy to fix.
Chris

I get the same error I got with libxcb-1.11.

[...]
_______________________________________________
Xcb mailing list
http://lists.freedesktop.org/mailman/listinfo/xcb

_______________________________________________
Xcb mailing list
http://lists.freedesktop.org/mailman/listinfo/xcb

aarzilli

2015-09-13 10:49:13 UTC

Permalink

Fields AuthorizationProtocolName and AuthorizationProtocolData of
SetupRequest should be padded:

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

The problem was discovered by github user pphaneuf while trying to use xgb
to write his own implementation of the connection handshake. Neither xgb
nor xcb actually use code generated for SetupRequest for the handshake,
which is why this bug went unnoticed.

https://github.com/BurntSushi/xgb/issues/24

Alessandro Arzilli.

---
src/xproto.xml | 2 ++
1 file changed, 2 insertions(+)

diff --git a/src/xproto.xml b/src/xproto.xml
index bfb8a4c..143318b 100644
--- a/src/xproto.xml
+++ b/src/xproto.xml
@@ -195,9 +195,11 @@ authorization from the authors.
<list type="char" name="authorization_protocol_name">
<fieldref>authorization_protocol_name_len</fieldref>
</list>
+ <pad align="4" />
<list type="char" name="authorization_protocol_data">
<fieldref>authorization_protocol_data_len</fieldref>
</list>
+ <pad align="4" />
</struct>

<struct name="SetupFailed">

--
1.9.1

Alessandro Arzilli

2017-01-23 16:21:36 UTC

Permalink

Any reason this was not merged for xcb-proto 1.12?

Alessandro Arzilli.

Post by aarzilli
Fields AuthorizationProtocolName and AuthorizationProtocolData of
http://www.x.org/releases/current/doc/xproto/x11protocol.html#Encoding::Connection_Setup
The problem was discovered by github user pphaneuf while trying to use xgb
to write his own implementation of the connection handshake. Neither xgb
nor xcb actually use code generated for SetupRequest for the handshake,
which is why this bug went unnoticed.
https://github.com/BurntSushi/xgb/issues/24
Alessandro Arzilli.
---
src/xproto.xml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/xproto.xml b/src/xproto.xml
index bfb8a4c..143318b 100644
--- a/src/xproto.xml
+++ b/src/xproto.xml
@@ -195,9 +195,11 @@ authorization from the authors.
<list type="char" name="authorization_protocol_name">
<fieldref>authorization_protocol_name_len</fieldref>
</list>
+ <pad align="4" />
<list type="char" name="authorization_protocol_data">
<fieldref>authorization_protocol_data_len</fieldref>
</list>
+ <pad align="4" />
</struct>
<struct name="SetupFailed">

Christian Linhart

2017-01-24 09:58:28 UTC

Permalink

Hi Alessandro,

Sorry for this patch being uncommitted for so long and thank you for the reminder.

Reasons for missing to include it in 1.12.:
None of us volunteers had the time to go through the list of
uncommitted patches in patchwork. And nobody else volunteered to do that.
We have some serious lack of people willing to do work for XCB. :-(

Second reason: version 1.12. had to be rushed out due to some critical bugfix
with respect to 64/32 bit issues.

I was completely away from the project for more than a year or so.
But I am returning. At least in the role of making some changes in the code.
See the patches that I have posted recently.

I'll push your patch soon.

Chris

P.S.: Do you want to volunteer for doing work for XCB? ;-)

Seriously, we need somebody doing secretary/assistant type of work
like checking for uncommitted patches that have a thumbs up from reviewers etc.

Any volunteers?

Post by Alessandro Arzilli
Any reason this was not merged for xcb-proto 1.12?
Alessandro Arzilli.

_______________________________________________
Xcb mailing list
https://lists.freedesktop.org/mailman/listinfo/xcb

Christian Linhart

2017-01-24 10:06:55 UTC

Permalink

I have checked this against the spec.

Therefore:
Reviewed-by: Christian Linhart <***@demorecorder.com>

Chris

Christian Linhart

2017-01-24 10:12:23 UTC

Permalink

I have just pushed this patch.

This patch had more than enough time from other potential reviewers
to comment on. ( in fact more than a year )

Sorry for the delay.

Chris

Post by Christian Linhart
I have checked this against the spec.
Chris

_______________________________________________
Xcb mailing list
https://lists.freedesktop.org/mailman/listinfo/xcb

Alessandro Arzilli

2014-09-19 06:57:37 UTC

Permalink

I get the same error I got with libxcb-1.11.

With this SetupRequest:

===
<struct name="SetupRequest">
<field type="CARD8" name="byte_order" />
<pad bytes="1" />
<field type="CARD16" name="protocol_major_version" />
<field type="CARD16" name="protocol_minor_version" />
<field type="CARD16" name="authorization_protocol_name_len" />
<field type="CARD16" name="authorization_protocol_data_len" />
<pad bytes="2" />
<list type="char" name="authorization_protocol_name">
<fieldref>authorization_protocol_name_len</fieldref>
</list>
<pad align="4" />
<list type="char" name="authorization_protocol_data">
<fieldref>authorization_protocol_data_len</fieldref>
</list>
<pad align="4" />
</struct>
===

I get this error:

===
xproto.c: In function 'xcb_setup_request_authorization_protocol_data_end':
xproto.c:629:36: error: 'None' undeclared (first use in this function)
xcb_generic_iterator_t child = None;
^
xproto.c:629:36: note: each undeclared identifier is reported only once for each
function it appears in
===

The error stays even if I remove the second "pad align". Just to be sure the
branch you wanted me to check out is ParametrizedStruct-V3, right?

Alessandro Arzilli.