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"

Reply via email to