svn commit: r210578 - head/usr.bin/grep
Jilles Tjoelker
jilles at stack.nl
Thu Jul 29 12:20:36 UTC 2010
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
More information about the svn-src-head
mailing list