From owner-svn-src-all@FreeBSD.ORG Thu Jul 29 12:20:36 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 220E0106566B; Thu, 29 Jul 2010 12:20:36 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) by mx1.freebsd.org (Postfix) with ESMTP id B453E8FC15; Thu, 29 Jul 2010 12:20:35 +0000 (UTC) Received: from turtle.stack.nl (turtle.stack.nl [IPv6:2001:610:1108:5010::132]) by mx1.stack.nl (Postfix) with ESMTP id C8EEB35A83D; Thu, 29 Jul 2010 14:20:34 +0200 (CEST) Received: by turtle.stack.nl (Postfix, from userid 1677) id B3BEF172BD; Thu, 29 Jul 2010 14:20:34 +0200 (CEST) Date: Thu, 29 Jul 2010 14:20:34 +0200 From: Jilles Tjoelker To: Gabor Kovesdan Message-ID: <20100729122034.GA28899@stack.nl> References: <201007290011.o6T0BE0l072516@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201007290011.o6T0BE0l072516@svn.freebsd.org> User-Agent: Mutt/1.5.20 (2009-06-14) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r210578 - head/usr.bin/grep X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Jul 2010 12:20:36 -0000 On Thu, Jul 29, 2010 at 12:11:14AM +0000, Gabor Kovesdan wrote: > Author: gabor > Date: Thu Jul 29 00:11:14 2010 > New Revision: 210578 > URL: http://svn.freebsd.org/changeset/base/210578 > Log: > - Some improvements on the exiting code, like replacing memcpy with > strlcpy/strcpy Hmm, I don't think this is an improvement :( If you know the length of the string, then memcpy() is usually the right function to use. I think this is clearer, and it is definitely more efficient. The strlcpy() function is for cases where the code does not know the length. The receiving buffer typically has a fixed length and overflow either leads to an error condition or silent truncation. (In the latter case, a security problem might still result.) Note that this is the reasoning of the glibc people why they do not want strlcat/strlcpy. I think these functions are acceptable but should be used with care, not as a panacea against all buffer overflows. > Modified: head/usr.bin/grep/fastgrep.c > ============================================================================== > --- head/usr.bin/grep/fastgrep.c Wed Jul 28 21:52:09 2010 (r210577) > +++ head/usr.bin/grep/fastgrep.c Thu Jul 29 00:11:14 2010 (r210578) > @@ -119,8 +119,7 @@ fastcomp(fastgrep_t *fg, const char *pat > * string respectively. > */ > fg->pattern = grep_malloc(fg->len + 1); > - memcpy(fg->pattern, pat + (bol ? 1 : 0) + wflag, fg->len); > - fg->pattern[fg->len] = '\0'; > + strlcpy(fg->pattern, pat + (bol ? 1 : 0) + wflag, fg->len + 1); > > /* Look for ways to cheat...er...avoid the full regex engine. */ > for (i = 0; i < fg->len; i++) { :( Note that this code may not be safe if fg->len comes from an untrusted user, as fg->len + 1 is 0 if fg->len == SIZE_MAX. This is not the case if fg->len is an actual length from strlen() or similar. > Modified: head/usr.bin/grep/grep.c > ============================================================================== > --- head/usr.bin/grep/grep.c Wed Jul 28 21:52:09 2010 (r210577) > +++ head/usr.bin/grep/grep.c Thu Jul 29 00:11:14 2010 (r210578) [snip] > @@ -234,32 +237,44 @@ add_pattern(char *pat, size_t len) > --len; > /* pat may not be NUL-terminated */ > pattern[patterns] = grep_malloc(len + 1); > - memcpy(pattern[patterns], pat, len); > - pattern[patterns][len] = '\0'; > + strlcpy(pattern[patterns], pat, len + 1); > ++patterns; > } :( Alternatively, consider strndup() here. > /* > - * Adds an include/exclude pattern to the internal array. > + * Adds a file include/exclude pattern to the internal array. > */ > static void > -add_epattern(char *pat, size_t len, int type, int mode) > +add_fpattern(const char *pat, int mode) > { > > /* Increase size if necessary */ > - if (epatterns == epattern_sz) { > - epattern_sz *= 2; > - epattern = grep_realloc(epattern, ++epattern_sz * > + if (fpatterns == fpattern_sz) { > + fpattern_sz *= 2; > + fpattern = grep_realloc(fpattern, ++fpattern_sz * > sizeof(struct epat)); > } > - if (len > 0 && pat[len - 1] == '\n') > - --len; > - epattern[epatterns].pat = grep_malloc(len + 1); > - memcpy(epattern[epatterns].pat, pat, len); > - epattern[epatterns].pat[len] = '\0'; > - epattern[epatterns].type = type; > - epattern[epatterns].mode = mode; > - ++epatterns; > + fpattern[fpatterns].pat = grep_strdup(pat); > + fpattern[fpatterns].mode = mode; > + ++fpatterns; > +} This part seems an improvement. The length came from strlen() anyway. > Modified: head/usr.bin/grep/queue.c > ============================================================================== > --- head/usr.bin/grep/queue.c Wed Jul 28 21:52:09 2010 (r210577) > +++ head/usr.bin/grep/queue.c Thu Jul 29 00:11:14 2010 (r210578) > @@ -60,7 +60,7 @@ enqueue(struct str *x) > item->data.len = x->len; > item->data.line_no = x->line_no; > item->data.off = x->off; > - memcpy(item->data.dat, x->dat, x->len); > + strcpy(item->data.dat, x->dat); > item->data.file = x->file; > > STAILQ_INSERT_TAIL(&queue, item, list); :( > Modified: head/usr.bin/grep/util.c > ============================================================================== > --- head/usr.bin/grep/util.c Wed Jul 28 21:52:09 2010 (r210577) > +++ head/usr.bin/grep/util.c Thu Jul 29 00:11:14 2010 (r210578) > @@ -39,6 +39,7 @@ __FBSDID("$FreeBSD$"); > #include > #include > #include > +#include > #include > #include > #include > @@ -51,6 +52,45 @@ __FBSDID("$FreeBSD$"); > static int linesqueued; > static int procline(struct str *l, int); > > +bool > +file_matching(const char *fname) > +{ > + bool ret; > + > + ret = finclude ? false : true; > + > + for (unsigned int i = 0; i < fpatterns; ++i) { > + if (fnmatch(fpattern[i].pat, > + fname, 0) == 0 || fnmatch(fpattern[i].pat, > + basename(fname), 0) == 0) { > + if (fpattern[i].mode == EXCL_PAT) > + return (false); > + else > + ret = true; > + } > + } > + return (ret); > +} > + > +bool > +dir_matching(const char *dname) > +{ > + bool ret; > + > + ret = dinclude ? false : true; > + > + for (unsigned int i = 0; i < dpatterns; ++i) { > + if (dname != NULL && > + fnmatch(dname, dpattern[i].pat, 0) == 0) { > + if (dpattern[i].mode == EXCL_PAT) > + return (false); > + else > + ret = true; > + } > + } > + return (ret); > +} > + > /* > * Processes a directory when a recursive search is performed with > * the -R option. Each appropriate file is passed to procfile(). > @@ -61,7 +101,6 @@ grep_tree(char **argv) > FTS *fts; > FTSENT *p; > char *d, *dir = NULL; > - unsigned int i; > int c, fts_flags; > bool ok; > > @@ -102,30 +141,19 @@ grep_tree(char **argv) > default: > /* Check for file exclusion/inclusion */ > ok = true; > - if (exclflag) { > + if (dexclude || dinclude) { > if ((d = strrchr(p->fts_path, '/')) != NULL) { > dir = grep_malloc(sizeof(char) * > (d - p->fts_path + 2)); > strlcpy(dir, p->fts_path, > (d - p->fts_path + 1)); Why do the buffer sizes differ here? Also, this can be a memcpy plus a '\0' write instead of a strlcpy. -- Jilles Tjoelker