In message: <20100729122034.ga28...@stack.nl>
Jilles Tjoelker <jil...@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
:
_______________________________________________
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"
