From owner-svn-src-all@FreeBSD.ORG Thu Jul 29 20:27: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 D30AD1065675; Thu, 29 Jul 2010 20:27:36 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 84D438FC15; Thu, 29 Jul 2010 20:27:36 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id o6TKJuWg013576; Thu, 29 Jul 2010 14:19:57 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Thu, 29 Jul 2010 14:20:18 -0600 (MDT) Message-Id: <20100729.142018.295937982770216853.imp@bsdimp.com> To: jilles@stack.nl From: "M. Warner Losh" In-Reply-To: <20100729122034.GA28899@stack.nl> References: <201007290011.o6T0BE0l072516@svn.freebsd.org> <20100729122034.GA28899@stack.nl> X-Mailer: Mew version 6.3 on Emacs 22.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, gabor@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 20:27:36 -0000 In message: <20100729122034.GA28899@stack.nl> Jilles Tjoelker writes: : 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 :( I agree in this case. The changes don't actually improve the safety of the code, but may make the code slower due to the need to recompute the length... : 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. The glibc people's line of reasoning is flawed. Because it might be abused, it should be excluded is not a good reason. It is demonstrably better than the alternatives, even if it doesn't solve all problems. By the same logic, one should remove most of str* functions, since they can all be misused... Warner : > 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 :