Re: [hackers] [sbase] [PATCH 1/3] find: Fix unterminated array in -ok primary
Quoth Evan Gates on Mon, Jun 15 2015 13:51 -0700:
On Mon, Jun 15, 2015 at 12:27 PM, Wolfgang Corcoran-Mathe
wrote:
---
find.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/find.c b/find.c
index dedf5a1..a870a90 100644
--- a/find.c
+++ b/find.c
@@ -429,6 +429,7 @@ pri_ok(struct arg *arg)
/* insert filename everywhere user gave us {} */
for (brace = o->braces; *brace; brace++)
**brace = arg->path;
+ *brace = NULL;
switch((pid = fork())) {
case -1:
--
2.3.5
Not needed there, *brace will already be NULL or the loop wouldn't
have stopped (the cause of the segfault). Needed in get_ok_arg()
so that it is NULL when we get here. See attached.
It occurred to me shortly after sending that patch that this should
have been in get_ok_arg(), as with get_exec_arg(). But without this
patch (or yours), I get a segfault when the end of the arg list is
reached. Your patch is obviously the right one, but I would like
to understand why both seem to have the same effect.
There is another semi-bug in -ok's input parsing. If the char read
into reply in pri_ok() at line 415 is a newline the user will have
to enter another newline to skip the file. This seems clumsy, but
it might be what you want.
Regards,
--
WCM
Re: [hackers] [sbase] [PATCH 1/3] find: Fix unterminated array in -ok primary
On Mon, Jun 15, 2015 at 12:27 PM, Wolfgang Corcoran-Mathe
wrote:
> ---
> find.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/find.c b/find.c
> index dedf5a1..a870a90 100644
> --- a/find.c
> +++ b/find.c
> @@ -429,6 +429,7 @@ pri_ok(struct arg *arg)
> /* insert filename everywhere user gave us {} */
> for (brace = o->braces; *brace; brace++)
> **brace = arg->path;
> + *brace = NULL;
>
> switch((pid = fork())) {
> case -1:
> --
> 2.3.5
>
>
Not needed there, *brace will already be NULL or the loop wouldn't
have stopped (the cause of the segfault). Needed in get_ok_arg() so
that it is NULL when we get here. See attached.
-emg
From d842da4c8f9fffd304858a0ae62f1c2933c97cef Mon Sep 17 00:00:00 2001
From: Evan Gates
Date: Mon, 15 Jun 2015 13:49:38 -0700
Subject: [PATCH] NULL terminate braces array in get_ok_arg
---
find.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/find.c b/find.c
index dcefca5..e9d8c1d 100644
--- a/find.c
+++ b/find.c
@@ -635,6 +635,7 @@ get_ok_arg(char *argv[], union extra *extra)
for (arg = argv, braces = o->braces; *arg; arg++)
if (!strcmp(*arg, "{}"))
*braces++ = arg;
+ *braces = NULL;
gflags.print = 0;
return arg;
--
2.4.3
Re: [hackers] [sbase] [PATCH] find
On Mon, Jun 15, 2015 at 12:27 PM, Wolfgang Corcoran-Mathe
wrote:
> emg,
>
> I noticed a few other things in get_ok_arg(), primarily that the fgets()
> flushing loop was causing find to hang mysteriously. Patch 2/3 should
> fix this, hopefully.
Should have said buf[strlen(buf) - 1] != '\n' in order to keep
reading until we got a full newline terminated line of input, not sure
how that got through. I prefer your solution as it doesn't run into
any of the '\0' problems.
> The isplus branch seems to work, based on limited testing.
Yep, it looks alright, the bug was specific to the array of pointers
to {} used in -ok and -exec \;
I had originally thought it was a bug relating to NULL termination for
execvp() when reading your mail (I finally have the code in front of
me, that's helpful).
> I've enjoyed reading your code, btw.
Thanks! Good to hear. Criticism, constructive or otherwise, is always welcome.
-emg
Re: [hackers] [sbase] [PATCH 2/3] find: Fix flushing input buffer with -ok
On Mon, Jun 15, 2015 at 12:27 PM, Wolfgang Corcoran-Mathe wrote: > emg's FIXME about nulls still applies. > > /* throw away rest of line */ > - while (fgets(buf, sizeof(buf), stdin) && *buf && buf[strlen(buf) - 1] > == '\n') > + while ((c = fgetc(stdin)) != '\n' && c != EOF) > /* FIXME: what if the first character of the rest of the line > is a null > -* byte? probably shouldn't juse fgets() */ > +* byte? */ > ; Nope, not a problem anymore as you're explicitly checking against '\n' and EOF so reading a '\0' is fine. Much better than the fgets. -emg
[hackers] [sbase] [PATCH 1/3] find: Fix unterminated array in -ok primary
---
find.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/find.c b/find.c
index dedf5a1..a870a90 100644
--- a/find.c
+++ b/find.c
@@ -429,6 +429,7 @@ pri_ok(struct arg *arg)
/* insert filename everywhere user gave us {} */
for (brace = o->braces; *brace; brace++)
**brace = arg->path;
+ *brace = NULL;
switch((pid = fork())) {
case -1:
--
2.3.5
[hackers] [sbase] [PATCH 2/3] find: Fix flushing input buffer with -ok
The original flush-stdin loop (with fgets()) hung until the user
entered some extraneous characters for it to kill.
emg's FIXME about nulls still applies.
---
find.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/find.c b/find.c
index a870a90..3871105 100644
--- a/find.c
+++ b/find.c
@@ -408,16 +408,16 @@ pri_ok(struct arg *arg)
{
int status, reply;
pid_t pid;
- char ***brace, buf[256];
+ char ***brace, c;
struct okarg *o = arg->extra.p;
fprintf(stderr, "%s: %s ?", *o->argv, arg->path);
reply = fgetc(stdin);
/* throw away rest of line */
- while (fgets(buf, sizeof(buf), stdin) && *buf && buf[strlen(buf) - 1]
== '\n')
+ while ((c = fgetc(stdin)) != '\n' && c != EOF)
/* FIXME: what if the first character of the rest of the line
is a null
-* byte? probably shouldn't juse fgets() */
+* byte? */
;
if (feof(stdin)) /* FIXME: ferror()? */
--
2.3.5
Re: [hackers] [sbase] [PATCH] find
emg, I noticed a few other things in get_ok_arg(), primarily that the fgets() flushing loop was causing find to hang mysteriously. Patch 2/3 should fix this, hopefully. The isplus branch seems to work, based on limited testing. I've enjoyed reading your code, btw. -- WCM
[hackers] [sbase] [PATCH 3/3] find: Improve prompt spacing with -ok
--- find.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/find.c b/find.c index 3871105..4ce150f 100644 --- a/find.c +++ b/find.c @@ -411,7 +411,7 @@ pri_ok(struct arg *arg) char ***brace, c; struct okarg *o = arg->extra.p; - fprintf(stderr, "%s: %s ?", *o->argv, arg->path); + fprintf(stderr, "%s: %s ? ", *o->argv, arg->path); reply = fgetc(stdin); /* throw away rest of line */ -- 2.3.5
Re: [hackers] [sbase] [PATCH] find: Fix unterminated array
On Mon, Jun 15, 2015 at 9:28 AM, Evan Gates wrote: > On Sat, Jun 13, 2015 at 10:42 PM, Wolfgang Corcoran-Mathe > wrote: >> This caused a segfault with semicolon-terminated -exec primaries. > > good catch, needs to be done in get_ok_arg() as well, not currently > somewhere I can do that, mind adding another patch? > -emg I'd recommend checking the isplus branch as well. If I screwed up the semicolon case chances are I screwed up the plus case, too. -emg
Re: [hackers] [sbase] [PATCH] find: Fix unterminated array
On Sat, Jun 13, 2015 at 10:42 PM, Wolfgang Corcoran-Mathe wrote: > This caused a segfault with semicolon-terminated -exec primaries. good catch, needs to be done in get_ok_arg() as well, not currently somewhere I can do that, mind adding another patch? -emg
Re: [hackers] st and combining characters
Hi,
I like the idea, but I think the patch needs some evolution.
A patch of 500 lines is usually hard of reading, and in in this case
the change is not trivial.
On Wed, Jun 10, 2015 at 11:39:25PM +0200, Joakim Sindholt wrote:
> glyph now holds a union of two combining characters and a pointer to a zero
> terminated array. This is purely to make the most of the space. No length is
> kept and every character added past 2 causes a realloc.
And why 2 characters?. I think it is more logical to have only one, which is
the most common case, isn't it?. Maybe I'm missing something.
> There's currently a problem with rendering where Xft doesn't draw the
> combining characters as a part of the character they combine to, which causes
> things like Hangul Jamo to be completely unusable stacks of characters instead
> of the proper combined form.
Uhmm, I don't understand this point, can you explain it a bit better?,
or better, put an example.
> @@ -98,6 +98,7 @@ enum glyph_attribute {
> ATTR_WRAP = 1 << 8,
> ATTR_WIDE = 1 << 9,
> ATTR_WDUMMY = 1 << 10,
> + ATTR_DECOMPPTR = 1 << 11,
> ATTR_BOLD_FAINT = ATTR_BOLD | ATTR_FAINT,
> };
>
> @@ -190,6 +191,10 @@ typedef XftColor Color;
>
> typedef struct {
> Rune u; /* character code */
> + union {
> + Rune c[2];
> + Rune *pc;
> + } dc;
We already have the field u, so why is not the union made with u and pc?
> Line *alt;/* alternate screen */
> bool *dirty; /* dirtyness of lines */
> XftGlyphFontSpec *specbuf; /* font spec buffer used for rendering */
> + int specbuflen;
> @@ -419,8 +426,9 @@ static void ttywrite(const char *, size_t);
> static void tstrsequence(uchar);
>
> static inline ushort sixd_to_16bit(int);
> -static int xmakeglyphfontspecs(XftGlyphFontSpec *, const Glyph *, int, int,
> int);
> -static void xdrawglyphfontspecs(const XftGlyphFontSpec *, Glyph, int, int,
> int);
> +static int xmakeglyphfontspecs(XftGlyphFontSpec *, int, const Glyph *, int,
> int, int);
> +static int xmakeglyphfontspecsintermbuf(const Glyph *, int, int, int);
> +static void xdrawglyphfontspecs(const XftGlyphFontSpec *, Glyph, int, int,
> int, int);
Uhmmm, these names begin to seem like java names. We must find a better way to
name them,
because with names so long the only lower case style doesn't work.
> /* append every set & selected glyph to the selection */
> @@ -984,6 +993,16 @@ getsel(void) {
> continue;
>
> ptr += utf8encode(gp->u, ptr);
> + if(gp->mode & ATTR_DECOMPPTR) {
> + pc = gp->dc.pc;
> + for(pc = gp->dc.pc; *pc; pc++)
> + ptr += utf8encode(*pc, ptr);
> + } else {
> + if(gp->dc.c[0])
> + ptr += utf8encode(gp->dc.c[0], ptr);
> + if(gp->dc.c[1])
> + ptr += utf8encode(gp->dc.c[1], ptr);
> + }
> }
If we define the union with u and pc we can remove the else in this code, no?
> + term.dirty[y] = 1;
> + if(term.line[y][x].mode & ATTR_DECOMPPTR) {
> + p = term.line[y][x].dc.pc;
> + while(*p)
> + p++;
> + sz = (p - term.line[y][x].dc.pc) + 2;
> + term.line[y][x].dc.pc = xrealloc(term.line[y][x].dc.pc, sz *
> sizeof(Rune));
> + term.line[y][x].dc.pc[sz-2] = u;
> + term.line[y][x].dc.pc[sz-1] = 0;
> + } else {
> + if(!term.line[y][x].dc.c[0]) {
> + term.line[y][x].dc.c[0] = u;
> + } else if(!term.line[y][x].dc.c[1]) {
> + term.line[y][x].dc.c[1] = u;
> + } else {
> + p = xmalloc(4 * sizeof(Rune));
> + p[0] = term.line[y][x].dc.c[0];
> + p[1] = term.line[y][x].dc.c[1];
> + p[2] = u;
> + p[3] = 0;
> + term.line[y][x].dc.pc = p;
> + term.line[y][x].mode |= ATTR_DECOMPPTR;
> + }
> + }
> +}
The same. If the union is made with u almost all off this code disappear.
> @@ -1711,7 +1786,7 @@ tclearregion(int x1, int y1, int x2, int y2) {
>
> void
> tdeletechar(int n) {
> - int dst, src, size;
> + int dst, src, size, i;
> Glyph *line;
>
> LIMIT(n, 0, term.col - term.c.x);
> @@ -1721,13 +1796,17 @@ tdeletechar(int n) {
> size = term.col - src;
> line = term.line[term.c.y];
>
> + for(i = dst; i < src; i++) {
> + if(line[i].mode & ATTR_DECOMPPTR)
> + line[i].dc.pc = NULL;
> + }
...
> @@ -1737,6 +1816,10 @@ tinsertblank(int n) {
> size = term.col - dst;
> line = term.line[term.
