Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IMdkit: Track window property offsets correctly #14

Closed
wants to merge 1 commit into from

Conversation

klemensbaum
Copy link

Problem symptoms

Input Methods relying on IMdkit for XIM support are susceptible to causing
hangs in X clients due to improper requests from the input method to the X server.

The following bug reports by users are related to this issue:

Detailed description

The XIM specification requires that XIM tranports over 20 bytes in size
be transferred via window properties. The sender calls XChangeProperty
with PropModeAppend, and instructs the recipient via ClientMessage to
call XGetWindowProperty with delete set to True.

Naive implementations exhibit a race condition because the receiver
could have written more data in the meantime, and XGetWindowProperty
only deletes the property when bytes_after_return is zero. If
bytes_after_return is non-zero, it is necessary to use an offset when
reading from the property again.

To ensure that the property data does not grow indefinitely, Xlib
recycles 21 Atoms in round-robin fashion. Because the XIM specification
does not limit the number of Atom names of the form "_clientXXX" to be
used for data transfer over window properties, an XIM server should be
able to keep of track any number of Atoms, remembering the offset into
each property.

This patch implements correct tracking of property offsets.

Signed-off-by: Klemens Baum klemensbaum@gmail.com
Reviewed-by: Keith Packard keithp@keithp.com

The XIM specification requires that XIM tranports over 20 bytes in size
be transferred via window properties. The sender calls XChangeProperty
with PropModeAppend, and instructs the recipient via ClientMessage to
call XGetWindowProperty with delete set to True.

Naive implementations exhibit a race condition because the receiver
could have written more data in the meantime, and XGetWindowProperty
only deletes the property when bytes_after_return is zero. If
bytes_after_return is non-zero, it is necessary to use an offset when
reading from the property again.

To ensure that the property data does not grow indefinitely, Xlib
recycles 21 Atoms in round-robin fashion. Because the XIM specification
does not limit the number of Atom names of the form "_clientXXX" to be
used for data transfer over window properties, an XIM server should be
able to keep of track any number of Atoms, remembering the offset into
each property.

This patch implements correct tracking of property offsets.

Signed-off-by: Klemens Baum <klemensbaum@gmail.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
@fujiwarat
Copy link
Member

Thank you for the patch.
It seems your patch fixes a hang in rxvt-unicode of https://code.google.com/p/ibus/issues/detail?id=1751 .
But https://code.google.com/p/ibus/issues/detail?id=1697 is not fixed.
I uploaded a test program:
https://fujiwara.fedorapeople.org/ibus/20141128/preedit_position.c
The prblem is typing "Window" does not give the right order.

@klemensbaum
Copy link
Author

I can reproduce the other bug with your test program, but it seems to be an issue with Xlib and would have to be fixed separately.

@fujiwarat
Copy link
Member

OK, I see.

@klemensbaum
Copy link
Author

Could you merge this pull request?

@fujiwarat
Copy link
Member

Yes, we're reviewing the patch.

@fujiwarat
Copy link
Member

I have a question.
The saving offset in_Xi18nSetPropertyOffset() does not depend on the value of bytes_after_ret from XGetWindowProperty() ?
In my test, bytes_after_ret is 88, 68, 44 and offset is 44. I.e. bytes_after_ret is sometimes greater than offset.

@klemensbaum
Copy link
Author

The offset only indicates how much of the property's data has already been consumed. It does not depend on bytes_after_ret other than being reset to 0 when the property is deleted (i.e. bytes_after_ret is 0).

@fujiwarat
Copy link
Member

Committed the patch.
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants