Bug 66345 - bad handling of overlong key name
Summary: bad handling of overlong key name
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: App/xkbcomp (show other bugs)
Version: git
Hardware: All All
: medium enhancement
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-29 00:40 UTC by Stéphane Aulery
Modified: 2013-08-28 23:41 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
correct handling of overlong keynames/strings (1.52 KB, patch)
2013-08-28 18:20 UTC, Benno Schulenberg
no flags Details | Splinter Review

Description Stéphane Aulery 2013-06-29 00:40:40 UTC
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
Comment 1 Peter Hutterer 2013-07-12 00:12:10 UTC
http://patchwork.freedesktop.org/patch/14036/
Comment 2 Peter Hutterer 2013-07-18 05:39:22 UTC
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)
Comment 3 Benno Schulenberg 2013-08-20 17:55:33 UTC
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;
}
Comment 4 Peter Hutterer 2013-08-28 05:43:06 UTC
(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
Comment 5 Benno Schulenberg 2013-08-28 18:20:34 UTC
Created attachment 84807 [details] [review]
correct handling of overlong keynames/strings

The requested patch.
Comment 6 Peter Hutterer 2013-08-28 23:41:38 UTC
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.