| Summary: | bad handling of overlong key name | ||||||
|---|---|---|---|---|---|---|---|
| Product: | xorg | Reporter: | Stéphane Aulery <saulery> | ||||
| Component: | App/xkbcomp | Assignee: | Xorg Project Team <xorg-team> | ||||
| Status: | RESOLVED FIXED | QA Contact: | Xorg Project Team <xorg-team> | ||||
| Severity: | enhancement | ||||||
| Priority: | medium | CC: | bensberg, peter.hutterer, saulery | ||||
| Version: | git | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| See Also: | http://bugs.debian.org/673031 | ||||||
| Whiteboard: | |||||||
| i915 platform: | i915 features: | ||||||
| Attachments: |
|
||||||
commit cdcd552041fc1325a2a81e3374fadb0dd15950dc Author: Peter Hutterer <peter.hutterer@who-t.net> Date: Thu Jul 11 13:26:18 2013 +1000 Always terminate the scanBuf string (#66345) Hmm. The fix doesn't seem fully correct to me. http://cgit.freedesktop.org/xorg/app/xkbcomp/commit/?id=cdcd552041fc1325a2a81e3374fadb0dd15950dc Shouldn't the line + if (i < sizeof(scanBuf) - i) read instead: + if (i < sizeof(scanBuf) - 1) ? But... when this if is executed, i *is* at the very most sizeof(scanBuf) - 1, so I don't see what the use of last is gaining us here. Wouldn't it be enough to just move the "scanBuf[i++] = '\0';" out of the next if statement? If so, maybe this should also be done for yyGetString? The relevant snippet from above commit: @@ -466,12 +467,20 @@ yyGetKeyName(void) if (i < sizeof(scanBuf) - 1) scanBuf[i++] = ch; } + + if (i < sizeof(scanBuf) - i) + last = i; + else + last = sizeof(scanBuf) - 1; + + scanBuf[last] = '\0'; + if ((ch == '>') && (i < 5)) { - scanBuf[i++] = '\0'; scanStrLine = lineNum; return KEYNAME; } (In reply to comment #3) > Shouldn't the line > + if (i < sizeof(scanBuf) - i) > read instead: > + if (i < sizeof(scanBuf) - 1) whoops, yes, you're right. > But... when this if is executed, i *is* at the very most sizeof(scanBuf) - 1, > so I don't see what the use of last is gaining us here. Wouldn't it be > enough > to just move the "scanBuf[i++] = '\0';" out of the next if statement? looks like I was overcomplicating things. you're right. please attach a patch for that > If so, maybe this should also be done for yyGetString? yep Created attachment 84807 [details] [review] correct handling of overlong keynames/strings The requested patch. commit 24d18e0a844041ef82441adb16aa18cc4b4814ae Author: Benno Schulenberg <bensberg@justemail.net> Date: Wed Aug 28 20:03:30 2013 +0200 Making sure that a copied string is always null-terminated (#66345). |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
XKB key names are limited to four octets. Initially I didn't know that, and tried to use longer names, like this: $ cat t0.xkb xkb_keymap { xkb_keycodes "test_keycodes" { minimum = 8; maximum = 255; <FOOBAR> = 10; }; xkb_types "complete" { include "complete" }; xkb_compatibility "complete" { include "complete" }; xkb_symbols "us" { include "us" }; xkb_geometry "kinesis" { include "kinesis" }; }; $ xkbcomp t0.xkb t0.xkm syntax error: line 5 of t0.xkb last scanned symbol is: FOOBARm Errors encountered in t0.xkb; not compiled. There's obviously a nul-termination problem in that error message. Possibly overrunning a fixed-size buffer too? -zefram