If PATH_MAX on your system is smaller than any path stored in the git

repository, that can cause memory corruption inside of the grep_tree

function used by git-grep.
Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>

---

 builtin-grep.c |    7 +++++--

 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin-grep.c b/builtin-grep.c

index ef29910..530a53d 100644

--- a/builtin-grep.c

+++ b/builtin-grep.c

@@ -441,14 +441,17 @@ static int grep_tree(struct grep_opt *opt, const char **paths,

 	len = strlen(path_buf);
 	while (tree_entry(tree, &entry)) {

-		strcpy(path_buf + len, entry.path);

+		int te_len = tree_entry_len(entry.path, entry.sha1);

+		if (len + te_len >= PATH_MAX + tn_len)

+			die ("path too long: %s", path_buf+tn_len);

+		memcpy(path_buf + len, entry.path, te_len);
 		if (S_ISDIR(entry.mode))

 			/* Match "abc/" against pathspec to

 			 * decide if we want to descend into "abc"

 			 * directory.

 			 */

-			strcpy(path_buf + len + tree_entry_len(entry.path, entry.sha1), "/");

+			strcpy(path_buf + len + te_len, "/");
 		if (!pathspec_matches(paths, down))

 			;

--

1.5.6.3.1.gb5587a
--

Hi,
That is brutal.  Does grep_tree() not work on tree objects in memory?  In

that case, you prevent the user from grepping, only because she is on a

suboptimal platform, _even if_ even that platform could cope with it.
It's not like the path is ever used to access a file, right?
Maybe you should convert the path_buf to a strbuf instead.
Ciao,

Dscho

--

If PATH_MAX on your system is smaller than any path stored in the git

repository, that can cause memory corruption inside of the grep_tree

function used by git-grep.
Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>

---

I have converted grep_tree code to use path_buf.
 builtin-grep.c |   26 ++++++++++++++------------

 1 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/builtin-grep.c b/builtin-grep.c

index ef29910..507bb95 100644

--- a/builtin-grep.c

+++ b/builtin-grep.c

@@ -427,33 +427,35 @@ static int grep_tree(struct grep_opt *opt, const char **paths,

 	struct name_entry entry;

 	char *down;

 	int tn_len = strlen(tree_name);

-	char *path_buf = xmalloc(PATH_MAX + tn_len + 100);

+	struct strbuf pathbuf;

+

+	strbuf_init(&pathbuf, PATH_MAX + tn_len);
 	if (tn_len) {

-		tn_len = sprintf(path_buf, "%s:", tree_name);

-		down = path_buf + tn_len;

-		strcat(down, base);

-	}

-	else {

-		down = path_buf;

-		strcpy(down, base);

+		strbuf_add(&pathbuf, tree_name, tn_len);

+		strbuf_addch(&pathbuf, ':');

+		tn_len = pathbuf.len;

 	}

-	len = strlen(path_buf);

+	strbuf_addstr(&pathbuf, base);

+	len = pathbuf.len;
 	while (tree_entry(tree, &entry)) {

-		strcpy(path_buf + len, entry.path);

+		int te_len = tree_entry_len(entry.path, entry.sha1);

+		pathbuf.len = len;

+		strbuf_add(&pathbuf, entry.path, te_len);
 		if (S_ISDIR(entry.mode))

 			/* Match "abc/" against pathspec to

 			 * decide if we want to descend into "abc"

 			 * directory.

 			 */

-			strcpy(path_buf + len + tree_entry_len(entry.path, entry.sha1), "/");

+			strbuf_addch(&pathbuf, '/');
+		down = pathbuf.buf + tn_len;

 		if (!pathspec_matches(paths, down))

 			;

 		else if (S_ISREG(entry.mode))

-			hit |= grep_sha1(opt, entry.sha1, path_buf, tn_len);

+			hit |= grep_sha1(opt, entry.sha1, pathbuf.buf, tn_len);

 		else if (S_ISDIR(entry.mode)) {

 			enum object_type type;

 			struct tree_desc sub;

--

1.5.6.3.1.g4e6bb
--

Sure, but other git commands do not work much better in this case.

In fact, what you called as "brutal" may be considered as very

polite comparing to what other git commands did.
For instance, git show will show you nothing at all and exit with 0.

The same problem with git whatchanged. The whole history mysteriously

disappeared at that commit, and git whatchanged exited with 0 without

any error or warning... Though git log will show you all history, but

if you run it with -p then it will also exit with zero at this commit

silently like previously history do not exist at all. So, I didn't see

any reason to make git grep to work in the situation where practically

any other git command does not. I guess, they should be corrected too,
It is probably a good suggestion, but I just wanted to provided a quick

fix to what may be considered as security issue. Of course, you usually

do not grep on untrusted repos, but if you did and something nasty

happened to you. I don't think it will help Git's reputation as being

secure and reliable...
Now the question is whether we really want to fix all Git commands that

do not touch the work tree to work with filenames longer than PATH_MAX?
Dmitry

--

It turned out that git actually crashed while doing diff, but because it

was a forked process, the parent exited normally with code 0. I wonder

should not the parent process to exit with a non-zero code if the child

died by SIGSEG or another signal?
Dmitry

--

If PATH_MAX on your system is smaller than a path stored, it may cause

buffer overflow and stack corruption in diff_addremove() and diff_change()

functions when running git-diff
Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>

---

 diff-lib.c  |    8 ++++----

 diff.c      |   11 ++---------

 diff.h      |    9 ++++-----

 revision.c  |    4 ++--

 tree-diff.c |   26 ++++++++++++++++++++++----

 5 files changed, 34 insertions(+), 24 deletions(-)
diff --git a/diff-lib.c b/diff-lib.c

index b17722d..e7eaff9 100644

--- a/diff-lib.c

+++ b/diff-lib.c

@@ -171,7 +171,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)

 			if (silent_on_removed)

 				continue;

 			diff_addremove(&revs->diffopt, '-', ce->ce_mode,

-				       ce->sha1, ce->name, NULL);

+				       ce->sha1, ce->name);

 			continue;

 		}

 		changed = ce_match_stat(ce, &st, ce_option);

@@ -184,7 +184,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)

 		newmode = ce_mode_from_stat(ce, st.st_mode);

 		diff_change(&revs->diffopt, oldmode, newmode,

 			    ce->sha1, (changed ? null_sha1 : ce->sha1),

-			    ce->name, NULL);

+			    ce->name);
 	}

 	diffcore_std(&revs->diffopt);

@@ -208,7 +208,7 @@ static void diff_index_show_file(struct rev_info *revs,

 				 const unsigned char *sha1, unsigned int mode)

 {

 	diff_addremove(&revs->diffopt, prefix[0], mode,

-		       sha1, ce->name, NULL);

+		       sha1, ce->name);

 }
 static int get_stat_data(struct cache_entry *ce,

@@ -312,7 +312,7 @@ static int show_modified(struct oneway_unpack_data *cbdata,

 		return 0;
 	diff_change(&revs->diffopt, oldmode, mode,

-		    old->sha1, sha1, old->name, NULL);

+		    old->sha1, sha1, old->name);

 	return 0;

 }
diff --git a/diff.c b/diff.c

index 78c4d3a..386de82 100644

--- a/diff.c

+++ b/diff.c

@@ -3356,9 +3356,8 @@ int diff_result_code(struct diff_options *opt, int status)

 void diff_addremove(...
[ message continues ]

If PATH_MAX on your system is smaller than a path stored in the git repo,

it may cause the buffer overflow in prepare_attr_stack.
Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>

---

 attr.c |   15 +++++++++------

 1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/attr.c b/attr.c

index 0fb47d3..73b6d6d 100644

--- a/attr.c

+++ b/attr.c

@@ -459,7 +459,9 @@ static void prepare_attr_stack(const char *path, int dirlen)

 {

 	struct attr_stack *elem, *info;

 	int len;

-	char pathbuf[PATH_MAX];

+	struct strbuf pathbuf;

+

+	strbuf_init(&pathbuf, dirlen+2+strlen(GITATTRIBUTES_FILE));
 	/*

 	 * At the bottom of the attribute stack is the built-in

@@ -510,13 +512,14 @@ static void prepare_attr_stack(const char *path, int dirlen)

 			len = strlen(attr_stack->origin);

 			if (dirlen <= len)

 				break;

-			memcpy(pathbuf, path, dirlen);

-			memcpy(pathbuf + dirlen, "/", 2);

-			cp = strchr(pathbuf + len + 1, '/');

+			pathbuf.len = 0;

+			strbuf_add(&pathbuf, path, dirlen);

+			strbuf_addch(&pathbuf, '/');

+			cp = strchr(pathbuf.buf + len + 1, '/');

 			strcpy(cp + 1, GITATTRIBUTES_FILE);

-			elem = read_attr(pathbuf, 0);

+			elem = read_attr(pathbuf.buf, 0);

 			*cp = '\0';

-			elem->origin = strdup(pathbuf);

+			elem->origin = strdup(pathbuf.buf);

 			elem->prev = attr_stack;

 			attr_stack = elem;

 			debug_push(elem);

--

1.5.6.3.3.gfcafb
--

+			strbuf_reset(&pathbuf);
-- Hannes

--

If PATH_MAX on your system is smaller than a path stored in the git repo,

it may cause the buffer overflow in prepare_attr_stack.
Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>

---
 attr.c |   15 +++++++++------

 1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/attr.c b/attr.c

index 0fb47d3..17f6a4d 100644

--- a/attr.c

+++ b/attr.c

@@ -459,7 +459,9 @@ static void prepare_attr_stack(const char *path, int dirlen)

 {

 	struct attr_stack *elem, *info;

 	int len;

-	char pathbuf[PATH_MAX];

+	struct strbuf pathbuf;

+

+	strbuf_init(&pathbuf, dirlen+2+strlen(GITATTRIBUTES_FILE));
 	/*

 	 * At the bottom of the attribute stack is the built-in

@@ -510,13 +512,14 @@ static void prepare_attr_stack(const char *path, int dirlen)

 			len = strlen(attr_stack->origin);

 			if (dirlen <= len)

 				break;

-			memcpy(pathbuf, path, dirlen);

-			memcpy(pathbuf + dirlen, "/", 2);

-			cp = strchr(pathbuf + len + 1, '/');

+			strbuf_reset(&pathbuf);

+			strbuf_add(&pathbuf, path, dirlen);

+			strbuf_addch(&pathbuf, '/');

+			cp = strchr(pathbuf.buf + len + 1, '/');

 			strcpy(cp + 1, GITATTRIBUTES_FILE);

-			elem = read_attr(pathbuf, 0);

+			elem = read_attr(pathbuf.buf, 0);

 			*cp = '\0';

-			elem->origin = strdup(pathbuf);

+			elem->origin = strdup(pathbuf.buf);

 			elem->prev = attr_stack;

 			attr_stack = elem;

 			debug_push(elem);

--

1.5.6.3.3.geccd
--

Hi,
We'll probably need to use the MinGW pager handling for Unix, too, and

check for a died child.
Not overly trivial, though.
Ciao,

Dscho
--