On Mon, Oct 24, 2022 at 09:21:37AM +0000, Andy Gozas wrote: > • XmbLookupString leaves the ksym unchanged if not filled and XLookupString > [probably] sets it to NoSymbol (that's what XLookupKeysym does, but whether > or not XLookupString shares this behavior is unclear [1]), so we can just > set it to NoSymbol in the beginning ourselves and check if it was changed > later
Initializing ksym to `NoSymbol` seems like a good idea. > • Since we can actually get the whole composed text when using > XmbLookupString by reallocating the buffer, I think we should do that — why > stop at 512 bytes? Mainly because using I think that the dynamic allocation patch made the control flow of the function more complicated than necessary. Backward goto is pretty bad in specific. But if you _do_ want to dynamically allocate, you only need to allocate right before buffer is being used. But which approach to take is the maintainer's call, not mine. I've attched both fixed-size and dynamic-allocation patch (but simplified without goto). - NRK
>From 6eae20945177667a0331670d0ca1c75226cfd29a Mon Sep 17 00:00:00 2001 From: NRK <[email protected]> Date: Mon, 24 Oct 2022 15:35:17 +0600 Subject: [PATCH] fix: buffer overflow when handling composed input XmbLookupString may leave buf as well as ksym uninitialized. initialize ksym to NoSymbol and track weather buffer was initialized or not via `got_buf`. Reported-by: Andy Gozas <[email protected]> --- x.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/x.c b/x.c index f70e3fb..b344dad 100644 --- a/x.c +++ b/x.c @@ -1846,20 +1846,24 @@ void kpress(XEvent *ev) { XKeyEvent *e = &ev->xkey; - KeySym ksym; - char buf[64], *customkey; + KeySym ksym = NoSymbol; + char buf[512], *customkey; int len; Rune c; Status status; const Shortcut *bp; + int got_buf = 0; if (IS_SET(MODE_KBDLOCK)) return; - if (xw.ime.xic) + if (xw.ime.xic) { len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, &ksym, &status); - else + got_buf = status == XLookupBoth || status == XLookupChars; + } else { len = XLookupString(e, buf, sizeof buf, &ksym, NULL); + got_buf = 1; + } /* 1. shortcuts */ for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) { if (ksym == bp->keysym && match(bp->mod, e->state)) { @@ -1875,7 +1879,7 @@ kpress(XEvent *ev) } /* 3. composed string from input method */ - if (len == 0) + if (len == 0 || !got_buf) return; if (len == 1 && e->state & Mod1Mask) { if (IS_SET(MODE_8BIT)) { -- 2.35.1
>From ea78eacf892ee232a5a060fa1204f896841f7ec5 Mon Sep 17 00:00:00 2001 From: NRK <[email protected]> Date: Mon, 24 Oct 2022 15:49:18 +0600 Subject: [PATCH] fix: buffer overflow when handling composed input XmbLookupString may leave buf as well as ksym uninitialized. initialize ksym to NoSymbol. track weather we need a larger buffer or not, and dynamically allocate if needed. Reported-by: Andy Gozas <[email protected]> --- x.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/x.c b/x.c index f70e3fb..e50ac90 100644 --- a/x.c +++ b/x.c @@ -1847,8 +1847,8 @@ kpress(XEvent *ev) { XKeyEvent *e = &ev->xkey; KeySym ksym; - char buf[64], *customkey; - int len; + char small_buf[512], *buf = small_buf, *customkey; + int len, buf_overflow = 0; Rune c; Status status; const Shortcut *bp; @@ -1856,10 +1856,12 @@ kpress(XEvent *ev) if (IS_SET(MODE_KBDLOCK)) return; - if (xw.ime.xic) + if (xw.ime.xic) { len = XmbLookupString(xw.ime.xic, e, buf, sizeof buf, &ksym, &status); - else + buf_overflow = status == XBufferOverflow ? len : 0; + } else { len = XLookupString(e, buf, sizeof buf, &ksym, NULL); + } /* 1. shortcuts */ for (bp = shortcuts; bp < shortcuts + LEN(shortcuts); bp++) { if (ksym == bp->keysym && match(bp->mod, e->state)) { @@ -1875,6 +1877,10 @@ kpress(XEvent *ev) } /* 3. composed string from input method */ + if (buf_overflow) { + buf = xmalloc(buf_overflow); + len = XmbLookupString(xw.ime.xic, e, buf, buf_overflow, &ksym, &status); + } if (len == 0) return; if (len == 1 && e->state & Mod1Mask) { @@ -1890,6 +1896,8 @@ kpress(XEvent *ev) } } ttywrite(buf, len, 1); + if (buf_overflow) + free(buf); } void -- 2.35.1
