Re: [hackers] [sbase][patch]find: copy path before using basename

2016-12-27 Thread Laslo Hunhold
On Wed, 5 Oct 2016 15:41:55 -0700
Evan Gates  wrote:

Hey Evan,

> On Mon, Oct 3, 2016 at 3:10 PM, FRIGN  wrote:
> > Please don't use VLA's. Use estrdup() in this case.
> 
> sbase-find_basename2.diff: revised patch without VLAs
> sbase-find_noVLAs.diff: path to remove all VLAs in find

thanks, applied with a few style modifications and using ereallocarray
() for overflow safety.

Cheers

Laslo

-- 
Laslo Hunhold 



Re: [hackers] [sbase][patch]find: copy path before using basename

2016-10-14 Thread Laslo Hunhold
On Wed, 5 Oct 2016 15:41:55 -0700
Evan Gates  wrote:

Hey Evan,

> On Mon, Oct 3, 2016 at 3:10 PM, FRIGN  wrote:
> > Please don't use VLA's. Use estrdup() in this case.
> 
> sbase-find_basename2.diff: revised patch without VLAs
> sbase-find_noVLAs.diff: path to remove all VLAs in find

let me give you some feedback on the patches.

### sbase-find_basename2.diff

 static int
 pri_name(struct arg *arg)
 {
-   return !fnmatch((char *)arg->extra.p, basename(arg->path), 0);
+   char *path = estrdup(arg->path);
+   int ret = !fnmatch((char *)arg->extra.p, basename(path), 0);
+   free(path);
+   return ret;
 }

I'd strictly separate declaration from initialization so it is easier
to see why we even need "ret" here.

static int
pri_name(struct arg *arg)
{
int ret;
char *path;

path = estrdup(arg->path);
ret = !fnmatch((char *)arg->extra.p, basename(path), 0);
free(path);

return ret;
}

### sbase-find_noVLAs.diff

This already looks good, but I really am a big fan of initializing
variables as late as possible.

+   struct tok *tok, *rpn, *out, **top,
+  *infix = emalloc((2 * argc + 1) * sizeof(*infix)),
+  **stack = emalloc(argc * sizeof(*stack));

Use ereallocarray() for cases like these, it also makes the code more
readable.
Thanks for your contributions!

Cheers

Laslo

-- 
Laslo Hunhold 



Re: [hackers] [sbase][patch]find: copy path before using basename

2016-10-05 Thread Evan Gates
On Mon, Oct 3, 2016 at 3:10 PM, FRIGN  wrote:
> Please don't use VLA's. Use estrdup() in this case.

sbase-find_basename2.diff: revised patch without VLAs
sbase-find_noVLAs.diff: path to remove all VLAs in find
From 47b19cf1c341627eb796469f3793e5c26baea767 Mon Sep 17 00:00:00 2001
From: Evan Gates 
Date: Wed, 5 Oct 2016 15:37:34 -0700
Subject: [PATCH] find: estrdup before basename

"The basename() function may modify the string pointed to by path..."
Thanks POSIX
---
 find.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/find.c b/find.c
index b331486..77f2935 100644
--- a/find.c
+++ b/find.c
@@ -229,7 +229,10 @@ static struct {
 static int
 pri_name(struct arg *arg)
 {
-   return !fnmatch((char *)arg->extra.p, basename(arg->path), 0);
+   char *path = estrdup(arg->path);
+   int ret = !fnmatch((char *)arg->extra.p, basename(path), 0);
+   free(path);
+   return ret;
 }
 
 static int
-- 
2.10.0

From 5163140694907a7070a7ffa8baec57014bde1dfd Mon Sep 17 00:00:00 2001
From: Evan Gates 
Date: Wed, 5 Oct 2016 15:34:52 -0700
Subject: [PATCH] find: remove VLAs

---
 find.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/find.c b/find.c
index 5f75735..b331486 100644
--- a/find.c
+++ b/find.c
@@ -770,7 +770,9 @@ find_op(char *name)
 static void
 parse(int argc, char **argv)
 {
-   struct tok infix[2 * argc + 1], *stack[argc], *tok, *rpn, *out, **top;
+   struct tok *tok, *rpn, *out, **top,
+  *infix = emalloc((2 * argc + 1) * sizeof(*infix)),
+  **stack = emalloc(argc * sizeof(*stack));
struct op_info *op;
struct pri_info *pri;
char **arg;
@@ -887,6 +889,9 @@ parse(int argc, char **argv)
 
toks = rpn;
root = *top;
+
+   free(infix);
+   free(stack);
 }
 
 /* for a primary, run and return result
@@ -925,8 +930,9 @@ find(char *path, struct findhist *hist)
DIR *dir;
struct dirent *de;
struct findhist *f, cur;
-   size_t len = strlen(path) + 2; /* null and '/' */
+   size_t namelen, pathcap = 0, len = strlen(path) + 2; /* null and '/' */
struct arg arg = { path, , { NULL } };
+   char *p, *pathbuf = NULL;
 
if ((gflags.l || (gflags.h && !hist) ? stat(path, ) : lstat(path, 
)) < 0) {
weprintf("failed to stat %s:", path);
@@ -968,18 +974,20 @@ find(char *path, struct findhist *hist)
}
 
while (errno = 0, (de = readdir(dir))) {
-   size_t pathcap = len + strlen(de->d_name);
-   char pathbuf[pathcap], *p;
-
if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, ".."))
continue;
-
+   namelen = strlen(de->d_name);
+   if (len + namelen > pathcap) {
+   pathcap = len + namelen;
+   pathbuf = erealloc(pathbuf, pathcap);
+   }
p = pathbuf + estrlcpy(pathbuf, path, pathcap);
if (*--p != '/')
estrlcat(pathbuf, "/", pathcap);
estrlcat(pathbuf, de->d_name, pathcap);
find(pathbuf, );
}
+   free(pathbuf);
if (errno) {
weprintf("readdir %s:", path);
closedir(dir);
-- 
2.10.0



Re: [hackers] [sbase][patch]find: copy path before using basename

2016-10-03 Thread FRIGN
On Mon, 3 Oct 2016 15:04:16 -0700
Evan Gates  wrote:

> "The basename() function may modify the string pointed to by path..."
> Thanks POSIX.

> + char path[strlen(arg->path)+1];
> + strcpy(path, arg->path);
> + return !fnmatch((char *)arg->extra.p, path, 0);

Please don't use VLA's. Use estrdup() in this case.

Cheers

FRIGN

-- 
FRIGN