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 <fnmatch.h> > #include <fts.h> > #include <libgen.h> > +#include <stdbool.h> > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > @@ -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 _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"