On Sat, Sep 21, 2013 at 03:41:01PM +0200, Mark Edgar wrote:
> This patch avoids a buffer overflow in kpress() if a key-mapped string
> longer than 31 characters is given in config.h. It also changes the
I think we should only check that the size of the string is the correct,
with something like:
@@ -3545,7 +3545,11 @@ kpress(XEvent *ev) {
/* 2. custom keys from config.h */
if((customkey = kmap(ksym, e->state))) {
- len = strlen(customkey);
+ if((len = strlen(customkey)) >= KEY_STR_SIZ) {
+ fprintf(stderr, "Incorrect key definition '%s'\n",
+ customkey);
+ return;
+ }
memcpy(buf, customkey, len);
/* 3. hardcoded (overrides X lookup) */
} else {
> maximum allowable key-mapped string length to a (likely) more
> reasonable 16.
I don't agree here, because if you insert a escape sequence is not dificult
get more of 16 characters (for example a key combination which updates the
bar title).
> +#define KEY_STR_SIZ 16
Good point. Using ESC_BUF_SIZ value was a bit tricky here, and define
a new size is a good point.
> #define CEIL(x) (((x) != (int) (x)) ? (x) + 1 : (x))
> +#define STRNLEN(s) ((s)[LEN((s)) - 1] == '\0' ? strlen((s)) : LEN((s)))
>
This trick is only useful when you usually have strings with a size of
LEN(s)-1, but in our case we will usually have string with a small size,
so it means we always will add a new test before of calling to strlen.
> @@ -3536,9 +3539,9 @@ kpress(XEvent *ev) {
> }
>
> /* 2. custom keys from config.h */
> - if((customkey = kmap(ksym, e->state))) {
> - len = strlen(customkey);
> - memcpy(buf, customkey, len);
> + if((kp = kmap(ksym, e->state))) {
> + len = STRNLEN(kp->s);
> + memcpy(buf, kp->s, len);
> /* 3. hardcoded (overrides X lookup) */
> } else {
> if(len == 0)
Could you explain what overflow are you fixing here?, because I only
can see you are returning the Key struct instead of returning directly
the char representation.
--
Roberto E. Vargas Caballero
----------------------------
k...@shike2.com
http://www.shike2.com
