Fix long compose result out-of-bounds write #114612

Phoenix-Katsch · 2023-11-08T01:56:05+01:00

Phoenix-Katsch commented

2023-11-08 01:56:05 +01:00

First-time contributor

xkb_compose_state_get_utf8 returns how many UTF-8 bytes the result would have taken up, excluding the NUL terminator, before truncation. When the compose result is over six bytes long, xkb_compose_state_feed_and_get_utf8 writes too many bytes into r_utf8_buf.

I've chosen to add a guard before memcpy, passing the keypress through unhandled instead. I don't see this changing much; this is an edge case anyway.

Note that the old code never wrote a NUL terminator to r_utf8_buf; this was never a bug because the buffer it wrote to was already initialized by the caller (keyboard_handle_key). I've changed the code to copy the entire buffer contents instead; I can't see this causing any issues.

I considered using strncpy (or equivalently: just changing the memcpy size) instead of adding a guard, but that could have cut Unicode codepoints in half and I don't know what ramifications that could have had; it seemed better to simply bail instead. With what little testing I did, it seems like key events are only expected to hold a single Unicode codepoint and simply ignore the rest of the buffer; so perhaps that would've been okay.

There are alternative approaches here. One way would be to expand a single compose sequence into multiple key events, one for each Unicode codepoint of the compose result.

Another way would be to instead change composition to send IME events, but it looks like these are behind the WITH_INPUT_IME feature flag, meaning that disabling IME would also disable composition unless the old code path is kept around.

`xkb_compose_state_get_utf8` returns how many UTF-8 bytes the result would have taken up, excluding the NUL terminator, *before* truncation. When the compose result is over six bytes long, `xkb_compose_state_feed_and_get_utf8` writes too many bytes into `r_utf8_buf`. I've chosen to add a guard before `memcpy`, passing the keypress through unhandled instead. I don't see this changing much; this is an edge case anyway. Note that the old code never wrote a NUL terminator to `r_utf8_buf`; this was never a bug because the buffer it wrote to was already initialized by the caller (`keyboard_handle_key`). I've changed the code to copy the entire buffer contents instead; I can't see this causing any issues. I considered using `strncpy` (or equivalently: just changing the `memcpy` size) instead of adding a guard, but that could have cut Unicode codepoints in half and I don't know what ramifications that could have had; it seemed better to simply bail instead. With what little testing I did, it seems like key events are only expected to hold a single Unicode codepoint and simply ignore the rest of the buffer; so perhaps that would've been okay. There are alternative approaches here. One way would be to expand a single compose sequence into multiple key events, one for each Unicode codepoint of the compose result. Another way would be to instead change composition to send IME events, but it looks like these are behind the `WITH_INPUT_IME` feature flag, meaning that disabling IME would also disable composition unless the old code path is kept around.

Phoenix-Katsch added 1 commit 2023-11-08 01:56:14 +01:00

aa71dc2882 Fix long compose result out-of-bounds write

`xkb_compose_state_get_utf8` returns how many UTF-8 bytes the
result would have taken up, excluding the NUL terminator, *before*
truncation. When the compose result is over six bytes long,
`xkb_compose_state_feed_and_get_utf8` writes too many bytes into
`r_utf8_buf`.

I've chosen to add a guard before `memcpy`, passing the keypress through
unhandled instead. I don't see this changing much; this is an edge case
anyway.

Note that the old code never wrote a NUL terminator to `r_utf8_buf`;
this was never a bug because the buffer it wrote to was already
initialized by the caller (`keyboard_handle_key`). I've changed the code
to copy the entire buffer contents instead; I can't see this causing any
issues.

I considered using `strncpy` instead of adding a guard, but that could
have cut Unicode codepoints in half and I don't know what ramifications
that could have had; it seemed better to simply bail instead. With what
little testing I did, it seems like key events are only expected to hold
a single Unicode codepoint and simply ignore the rest of the buffer; so
perhaps that would've been okay.

There are alternative approaches here. One way would be to expand a
single compose sequence into multiple key events, one for each Unicode
codepoint of the compose result.

Another way would be to instead change composition to send IME events,
but it looks like these are behind the `WITH_INPUT_IME` feature flag,
meaning that disabling IME would also disable composition unless the old
code path is kept around.

Iliya Katushenock added the

Module

Core

label 2023-11-08 06:25:19 +01:00

Merge conflict checking is in progress. Try again in few moments.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u fix-compose-oob-write:Phoenix-Katsch-fix-compose-oob-write

git checkout Phoenix-Katsch-fix-compose-oob-write

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix long compose result out-of-bounds write #114612

Checkout