svn commit: r210578 - head/usr.bin/grep

M. Warner Losh imp at bsdimp.com
Thu Jul 29 20:27:36 UTC 2010


In message: <20100729122034.GA28899 at stack.nl>
            Jilles Tjoelker <jilles at stack.nl> 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 <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
: 


More information about the svn-src-all mailing list