buffer overflow when executing 'xkbcomp -C :0'

Created an attachment (id=21847)
Proposed method for fixing overflow situation

Forwarding this bug report from a Ubuntu user:
https://bugs.edge.launchpad.net/ubuntu/+source/x11-xkb-utils/+bug/309013

[Problem]
Ubuntu is now using the fortify-source gcc compilation options to help find potential buffer overflows, and found the following in the xkbcomp code.

#0 0x00007f6c4a726f85 in raise () from /lib/libc.so.6
No symbol table info available.
#1 0x00007f6c4a728af3 in abort () from /lib/libc.so.6
No symbol table info available.
#2 0x00007f6c4a766218 in ?? () from /lib/libc.so.6
No symbol table info available.
#3 0x00007f6c4a7f3307 in __fortify_fail () from /lib/libc.so.6
No symbol table info available.
#4 0x00007f6c4a7f11b0 in __chk_fail () from /lib/libc.so.6
No symbol table info available.
#5 0x00007f6c4aa6cad5 in WriteCHdrKeyTypes (file=0x19f1410, dpy=0x0, xkb=0x19e58d0) at /usr/include/bits/string3.h:106
 i = 22
 n = 3
 type = (XkbKeyTypePtr) 0x19e5d70
 prefix = "SEPARATE_CAPS_AND_SHIFT_ALPHABET"
#6 0x00007f6c4aa6dbe2 in WriteCHdrKeymap (file=0x19f1410, result=<value optimized out>) at ../../src/cout.c:335
 ok = <value optimized out>
 xkb = (XkbDescPtr) 0x19e58d0
#7 0x00007f6c4aa6ab9e in XkbWriteCFile (out=0x19f1410, name=0x19f165a "", result=0x7fff531b15d0) at ../../src/cout.c:1027
 tmp = 0x19f165a ""
 hdrdef = <value optimized out>
 ok = <value optimized out>
 xkb = (XkbDescPtr) 0x19e58d0
 func = (int (*)(FILE *, XkbFileInfo *)) 0x7f6c4aa6db30 <WriteCHdrKeymap>

Warning: Could not load keyboard geometry for :0
                  BadAlloc (insufficient resources for operation)
                  Resulting keymap file will not describe geometry
*** buffer overflow detected ***: xkbcomp terminated
======= Backtrace: =========
/lib/libc.so.6(__fortify_fail+0x37)[0x7f3bd4803307]
/lib/libc.so.6[0x7f3bd48011b0]
/usr/lib/libxkbfile.so.1[0x7f3bd4a7cad5]
/usr/lib/libxkbfile.so.1[0x7f3bd4a7dbe2]
/usr/lib/libxkbfile.so.1(XkbWriteCFile+0x26e)[0x7f3bd4a7ab9e]
xkbcomp[0x41d566]
/lib/libc.so.6(__libc_start_main+0xe6)[0x7f3bd4722586]
xkbcomp[0x4027f9]
======= Memory map: ========
00400000-0042e000 r-xp 00000000 08:01 8233554 /usr/bin/xkbcomp
0062e000-00630000 rw-p 0002e000 08:01 8233554 /usr/bin/xkbcomp
00630000-00631000 rw-p 00630000 00:00 0
00a54000-00a9b000 rw-p 00a54000 00:00 0 [heap]
7f3bd3ac2000-7f3bd3ad8000 r-xp 00000000 08:01 4907635 /lib/libgcc_s.so.1
7f3bd3ad8000-7f3bd3cd8000 ---p 00016000 08:01 4907635 /lib/libgcc_s.so.1
7f3bd3cd8000-7f3bd3cd9000 r--p 00016000 08:01 4907635 /lib/libgcc_s.so.1
7f3bd3cd9000-7f3bd3cda000 rw-p 00017000 08:01 4907635 /lib/libgcc_s.so.1
7f3bd3cda000-7f3bd3cdf000 r-xp 00000000 08:01 8235629 /usr/lib/libXdmcp.so.6.0.0
7f3bd3cdf000-7f3bd3ede000 ---p 00005000 08:01 8235629 /usr/lib/libXdmcp.so.6.0.0
7f3bd3ede000-7f3bd3edf000 rw-p 00004000 08:01 8235629 /usr/lib/libXdmcp.so.6.0.0
7f3bd3edf000-7f3bd3ee1000 r-xp 00000000 08:01 8233819 /usr/lib/libXau.so.6.0.0
7f3bd3ee1000...

Created an attachment (id=21848)
handles other crashing strcpy's

patch looks good in principle, minus the problem that asprintf isn't available on at least Solaris. Can you re-do the patch avoiding the use of asprintf please? (and while you're at it, a git-formatted patch would be nice :)

I used asprintf because I have no idea what the maximum length of the returned string from XkbAtomText is. I assume you could fix this issue using a corrected maximum size to the local stack buffer. How long can the result string be expected to be?

> I used asprintf because I have no idea what the maximum length of the returned
> string from XkbAtomText is. I assume you could fix this issue using a
> corrected maximum size to the local stack buffer. How long can the result
> string be expected to be?

shouldn't something like
--
char* foo = XkbAtomText(..);
char* bar = malloc(strlen(foo));
strcpy(bar, foo);
--
do the same job as asprintf?

Created an attachment (id=21912)
use strdup

Argh. Sorry, I've been debugging sprintf issues in other code for so long that I forgot about strdup. Duh. This should be portable.

getting there. instead of perror it's probably better to use _XkbLibError. and exit(1) in a library is a big no-no. Just return 0.
(yes, I know, I should have caught that in the first review)

Created an attachment (id=21913)
use XkbLibError, don't exit

How about this? Looks like WriteTypeInitFunc is void, so there's nothing to do but return after the error report.

> How about this? Looks like WriteTypeInitFunc is void, so there's nothing to do
> but return after the error report.

I think it' be better to just break out of the loop, since otherwise we leave
a dangling { in the output. The same with the rest of it, leaving unclosed {
around is probably bad. (I'm not sure how much that'll affect it though since
the output will be mostly empty anyway.)

Other than that, fine with me. Please attach a git-formatted patch so I can
push it easily.

Created an attachment (id=21956)
break from loop after writing out an #error

In an effort to put errors somewhere when the alloc fails, we can just write out an error to the C file, and continue the loop. Attach as a git patch.

Pushed as dd9514fe714d81b881a1bd6bd88d4287adc5fc7e. Thanks.

(Now, if you have the time to figure out why the geometry still has BadAllocs in some cases, that'd be much appreciated :)