Re: [dev] [st] [PATCH] Avoid buffer overflows in the case of key-mapped strings.
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
Re: [dev] [st] [patch] multi-line selection causes core dump on OpenBSD
> I'm running OpenBSD 5.3 amd64 release version with the most current st > version from git and I noticed that st was crashing and dumping core > when selecting multiple lines whith the cursor. This happens, because > on line 964 of st.c (gp-1)->mode is accessed, although gp is still Good catch. I am using also OpenBSD and I have noticied the problem and it was in my queue ;). > pointing at the beginning of the array term.line[y] (see line 939 for > initialization of gp). A patch follows at the end of the mail. I > _quickly_ tested it and it _seems_ to work, but this should be verified > by someone else, since I don't know the st code and I only came up with > it by inspecting the core dump. The patch is correct, but I think there is a way easier: - if(y < sel.ne.y && !((gp-1)->mode & ATTR_WRAP)) + if(y < sel.ne.y && x > 0 && !((gp-1)->mode & ATTR_WRAP)) If you agree with this small difference I will commit your change. Best regard. -- Roberto E. Vargas Caballero k...@shike2.com http://www.shike2.com
Re: [dev] [st] [patch] multi-line selection causes core dump on OpenBSD
On Mon, Sep 23, 2013 at 10:50 AM, Roberto E. Vargas Caballero wrote: >> I'm running OpenBSD 5.3 amd64 release version with the most current st >> version from git and I noticed that st was crashing and dumping core >> when selecting multiple lines whith the cursor. This happens, because >> on line 964 of st.c (gp-1)->mode is accessed, although gp is still > > Good catch. I am using also OpenBSD and I have noticied the problem > and it was in my queue ;). > > >> pointing at the beginning of the array term.line[y] (see line 939 for >> initialization of gp). A patch follows at the end of the mail. I >> _quickly_ tested it and it _seems_ to work, but this should be verified >> by someone else, since I don't know the st code and I only came up with >> it by inspecting the core dump. > > The patch is correct, but I think there is a way easier: > > - if(y < sel.ne.y && !((gp-1)->mode & ATTR_WRAP)) > + if(y < sel.ne.y && x > 0 && !((gp-1)->mode & > ATTR_WRAP)) > logically, wouldn't that be if(y < sel.ne.y && (x == 0 || !((gp-1)->mode & ATTR_WRAP))) ? I think the outcome for x==0 would be different in your suggestion. cheers! mar77i
Re: [dev] [st] [patch] multi-line selection causes core dump on OpenBSD
Otoh, don't prefix the first line with a newline. I seem to have been early there. :-) cheers! mar77i
Re: [dev] [st] [patch] multi-line selection causes core dump on OpenBSD
Roberto E. Vargas Caballero wrote: > If you agree with this small difference I will commit your change. I don't mind. Martti Kühne wrote: > logically, wouldn't that be > > if(y < sel.ne.y && (x == 0 || !((gp-1)->mode & ATTR_WRAP))) > > ? > > I think the outcome for x==0 would be different in your suggestion. Logically, it wouldn't be the same. In your case, "*ptr++ = '\n';" will be executed, regardless whether !((gp-1)->mode & ATTR_WRAP)) holds true or not (assuming x == 0). Whereas in Roberto's case, the if condition never holds true for x == 0. So as you mentioned, the outcome for x == 0 is different. It is to decide which one is the correct behaviour. As I my mentioned, a quick test of my fix (which should be logically equivalent to Roberto's fix) showed no problems, but then again, it was just a quick test. Best regards Maurice Quennet
[dev] [surf] New webkit engine worth keeping an eye on
Hi folks, I just found out about a new webkit port called "WebKitNix". It looks like a better alternative to WebKitGTK+, using OpenGL for rendering, and not requiring a UI toolkit. Initial announcement: http://article.gmane.org/gmane.os.opendarwin.webkit.devel/24877/ Website: http://webkitnix.openbossa.org/ Code: https://github.com/WebKitNix There's still plenty of suck, but definitely a good deal less than WebKitGTK+, and it looks to be getting better. It would be lovely to see surf switch to using this port at some point. Perhaps I will have time to look into doing this, perhaps not, we'll see. Nick
Re: [dev] [surf] New webkit engine worth keeping an eye on
> There's still plenty of suck, but definitely a good deal less than > WebKitGTK+, and it looks to be getting better. > > It would be lovely to see surf switch to using this port at some > point. Perhaps I will have time to look into doing this, perhaps > not, we'll see. It would be really good. Using WebKitGtk+ usually means install a lot of suck software, like for example dbus. This new port of WebKit can reduce a lot the number of dependencies of surf. -- Roberto E. Vargas Caballero k...@shike2.com http://www.shike2.com
Re: [dev] [surf] New webkit engine worth keeping an eye on
On Mon, Sep 23, 2013 at 05:19:44PM +0200, Roberto E. Vargas Caballero wrote: > It would be really good. Using WebKitGtk+ usually means install a lot > of suck software, like for example dbus. This new port of WebKit > can reduce a lot the number of dependencies of surf. Still WebKitNix requires GTK+3 to build. Hope it is only needed for jhbuild they use instead of make. -- Dmitrij D. Czarkoff
Re: [dev] st: bracketed paste mode
Greetings! - User Roberto E. Vargas Caballero on 2013-09-19 11:27:56 wrote: >It seems there are some users that could be interested in this feature, >so I will apply it next week. I have tested the latest master branch of st just to see what this feature is. It is really great! Thanks Egmont for this patch! I have never heard of this terminal's paste mode before, but under Vim it behaves perfectly. One more keyboard's key is free now. -- Happy hacking, Sergey Matveev
Re: [dev] [st] [patch] multi-line selection causes core dump on OpenBSD
On Mon, Sep 23, 2013 at 12:07:06PM +0200, Maurice Quennet wrote: > Roberto E. Vargas Caballero wrote: > > If you agree with this small difference I will commit your change. > > I don't mind. Applied. -- Roberto E. Vargas Caballero k...@shike2.com http://www.shike2.com
