threads / patch / 10125

patchMust not modify the_index.cache as it may be passed to realloc at some point.

Subject: [PATCH] Must not modify the_index.cache as it may be passed to realloc at some point.

## tl;dr

14 messages between Oct 3, 2007 and Oct 3, 2007. Diffs are folded; open one to read it.

replies: 13people: 7as markdown or json

Keith Packard· Oct 3, 2007, 05:44 UTC · lore

The index cache is not static, growing as new entries are added. If entries are added after prune_cache is called, cache will no longer point at the base of the allocation, and realloc will not be happy.

I verified that this was the only place in the current source which modified any index_state.cache elements aside from the alloc/realloc calls in read-cache by changing the type of the element to 'struct cache_entry ** const cache' and recompiling.

A more efficient patch would create a separate 'cache_base' value to track
the allocation and then fix things up when reallocation was necessary,
instead of the brute-force memmove used here.
---
 builtin-ls-files.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin-ls-files.c +1 −1
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 6c1db86..0028b8a 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -280,7 +280,7 @@ static void prune_cache(const char *prefix)
 
        if (pos < 0)
                pos = -pos-1;
-       active_cache += pos;
+       memmove (active_cache, active_cache + pos, (active_nr - pos) * sizeof (struct cache_entry *));
        active_nr -= pos;
        first = 0;
        last = active_nr;

-- 
1.5.3.3.131.g34c6d-dirty

-- 
keith.packard@intel.com
Junio C Hamano· Oct 3, 2007, 05:55 UTC · re: Keith Packard · lore

Re: [PATCH] Must not modify the_index.cache as it may be passed to realloc at some point.

Keith Packard <keithp@keithp.com> writes:
> The index cache is not static, growing as new entries are added. If entries
> are added after prune_cache is called, cache will no longer point at the
> base of the allocation, and realloc will not be happy.

Thanks for catching this. This code originally was perfectly Ok, but I broke it with the overlay_tree() change.

Carl Worth· Oct 3, 2007, 07:03 UTC · re: Junio C Hamano · lore

[PATCH] Add test case for ls-files --with-head

This tests basic functionality and also exercises a bug noticed
by Keith Packard, (prune_cache followed by add_index_entry can
trigger an attempt to realloc a pointer into the middle of an
allocated buffer).
---
 t/t3060-ls-files-with-head.sh |   53 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)
 create mode 100755 t/t3060-ls-files-with-head.sh
 On Tue, 02 Oct 2007 22:55:57 -0700, Junio C Hamano wrote:
 >
 > Thanks for catching this.  This code originally was perfectly
 > Ok, but I broke it with the overlay_tree() change.
 Yeah, Keith and I were really scratching our heads as to how this
 hadn't caused more problems earlier. I wrote the test case below to
 explore the issue and found the recent overlay_tree change as you
 mention, and that made more sense.
 I didn't notice any existing --with-tree test case, so perhaps the
 patch below is useful.
 -Carl
Show changes to t/t3060-ls-files-with-head.sh +53 −1
diff --git a/t/t3060-ls-files-with-head.sh b/t/t3060-ls-files-with-head.sh
new file mode 100755
index 0000000..4ead08b
--- /dev/null
+++ b/t/t3060-ls-files-with-head.sh
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Carl D. Worth
+#
+
+test_description='git ls-files test (--with-head).
+
+This test runs git ls-files --with-head and in particular in
+a scenario known to trigger a crash with some versions of git.
+'
+. ./test-lib.sh
+
+# The bug we're exercising requires a fair number of entries in a
+# sub-directory so that add_index_entry will trigger a realloc
+echo file > expected
+mkdir sub
+for num in $(seq -f%04g 1 50); do
+	touch sub/file-$num
+	echo file-$num >> expected
+done
+git add .
+git commit -m "add a bunch of files"
+
+# We remove them all so that we'll have something to add back with
+# --with-head and so that we'll definitely be under the realloc size
+# to trigger the bug.
+rm -r sub
+git commit -a -m "remove them all"
+
+# The bug also requires some entry before our directory so that
+# prune_path will modify the_index.cache
+mkdir a_directory_that_sorts_before_sub
+touch a_directory_that_sorts_before_sub/file
+mkdir sub
+touch sub/file
+git add .
+
+# We have to run from a sub-directory to trigger prune_path
+cd sub
+
+# Then we finally get to run our --with-tree test
+test_expect_success \
+    'git -ls-files --with-tree should succeed.' \
+    'git ls-files --with-tree=HEAD~1 >../output'
+
+cd ..
+test_expect_success \
+    'git -ls-files --with-tree should add entries from named tree.' \
+    'diff output expected'
+
+test_done
+
+
--
1.5.3.3.131.g34c6d
Johannes Sixt· Oct 3, 2007, 12:09 UTC · re: Carl Worth · lore

Re: [PATCH] Add test case for ls-files --with-head

Carl Worth schrieb:
> +for num in $(seq -f%04g 1 50); do
> +	touch sub/file-$num
> +	echo file-$num >> expected
> +done
seq is not universally available. Can we have that as
for i in 0 1 2 3 4; do
	for j in 0 1 2 3 4 5 6 7 8 9; do
		> sub/file-$i$j
		echo file-$i$j >> expected
	done
done
-- Hannes
Johannes Schindelin· Oct 3, 2007, 15:36 UTC · re: Johannes Sixt · lore

Re: [PATCH] Add test case for ls-files --with-head

Hi,
On Wed, 3 Oct 2007, Johannes Sixt wrote:
Show 14 quoted lines
> Carl Worth schrieb:
> > +for num in $(seq -f%04g 1 50); do
> > +	touch sub/file-$num
> > +	echo file-$num >> expected
> > +done
> 
> seq is not universally available. Can we have that as
> 
> for i in 0 1 2 3 4; do
> 	for j in 0 1 2 3 4 5 6 7 8 9; do
> 		> sub/file-$i$j
> 		echo file-$i$j >> expected
> 	done
> done
Or as
	i=1
	while test $i -le 50
	do
		num=$(printf %04d $i)
		> sub/file-$num
		echo file-$num >> expected
		i=$(($i+1))
	done

This version should be as portable, with the benefit that it is easier to change for different start and end values.

Ciao, Dscho

David Kastrup· Oct 3, 2007, 15:52 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Add test case for ls-files --with-head

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 23 quoted lines
> On Wed, 3 Oct 2007, Johannes Sixt wrote:
>> 
>> seq is not universally available. Can we have that as
>> 
>> for i in 0 1 2 3 4; do
>> 	for j in 0 1 2 3 4 5 6 7 8 9; do
>> 		> sub/file-$i$j
>> 		echo file-$i$j >> expected
>> 	done
>> done
>
> Or as
>
> 	i=1
> 	while test $i -le 50
> 	do
> 		num=$(printf %04d $i)
> 		> sub/file-$num
> 		echo file-$num >> expected
> 		i=$(($i+1))
> 	done
>
> This version should be as portable,

Huh? It uses the conceivably-not-builtin "test" (something which _you_ picked as something to complain about in a patch of mine where it was not used in an inner loop) on every iteration, it uses printf and it uses $((...)) arithmetic expansion. Whereas the proposal by Johannes works fine even on prehistoric shell versions. So the "as portable" enough moniker is surely weird.

> with the benefit that it is easier to change for different start and
> end values.
Correct.  But why would we want those here?
-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Carl Worth· Oct 3, 2007, 16:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Add test case for ls-files --with-head

On Wed, 3 Oct 2007 16:36:13 +0100 (BST), Johannes Schindelin wrote:
Show 5 quoted lines
> Or as
>
> 	i=1
> 	while test $i -le 50
> 	do
...
> 		i=$(($i+1))
> 	done
/me steps aside to let the shell-script wizards finish the job
-Carl
David Kastrup· Oct 3, 2007, 16:15 UTC · re: Carl Worth · lore

Re: [PATCH] Add test case for ls-files --with-head

Carl Worth <cworth@cworth.org> writes:
Show 11 quoted lines
> On Wed, 3 Oct 2007 16:36:13 +0100 (BST), Johannes Schindelin wrote:
>> Or as
>>
>> 	i=1
>> 	while test $i -le 50
>> 	do
> ...
>> 		i=$(($i+1))
>> 	done
>
> /me steps aside to let the shell-script wizards finish the job
for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}
do
  ...
done
There is enough room for perversion in shell programming for everyone...
-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Jeff King· Oct 3, 2007, 20:21 UTC · re: David Kastrup · lore

Re: [PATCH] Add test case for ls-files --with-head

On Wed, Oct 03, 2007 at 06:15:56PM +0200, David Kastrup wrote:
> for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}
> do
>   ...
> done

$ dash $ for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}; do echo $i; done {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9} $

-Peff
Johannes Schindelin· Oct 3, 2007, 21:39 UTC · re: Jeff King · lore

Re: [PATCH] Add test case for ls-files --with-head

Hi,
On Wed, 3 Oct 2007, Jeff King wrote:
Show 9 quoted lines
> > for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}
> > do
> >   ...
> > done
> 
> $ dash
> $ for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}; do echo $i; done
> {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}
> $

AFAIK this is the same as bash (I thought I was the last one to make that mistake 10 years ago). As long as you do not have _files_ matching the pattern, it does not expand. And besides, this is too complicated anyway: [1-5] is much shorter than {1,2,3,4,5}.

Ciao, Dscho

Junio C Hamano· Oct 3, 2007, 21:47 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Add test case for ls-files --with-head

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
>> $ for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}; do echo $i; done
>> {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}
>> $
>
> AFAIK this is the same as bash (I thought I was the last one to make that 
> mistake 10 years ago).  As long as you do not have _files_ matching the 
> pattern, it does not expand.  And besides, this is too complicated anyway: 
> [1-5] is much shorter than {1,2,3,4,5}.
AFAIK, you are wrong ;-)

{1,2,3,4,5} expands regardless of what's on the filesystem but I do not think it is POSIX.

[1-5] matches if any of the {1,2,3,4,5} is found on the filesystem.

Jeff King· Oct 3, 2007, 22:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add test case for ls-files --with-head

On Wed, Oct 03, 2007 at 02:47:01PM -0700, Junio C Hamano wrote:
> AFAIK, you are wrong ;-)
> 
> {1,2,3,4,5} expands regardless of what's on the filesystem but I
> do not think it is POSIX.
Yes, I think that is right.
-Peff
Junio C Hamano· Oct 3, 2007, 19:29 UTC · re: Carl Worth · lore

Re: [PATCH] Add test case for ls-files --with-head

Carl Worth <cworth@cworth.org> writes:
Show 11 quoted lines
> On Wed, 3 Oct 2007 16:36:13 +0100 (BST), Johannes Schindelin wrote:
>> Or as
>>
>> 	i=1
>> 	while test $i -le 50
>> 	do
> ...
>> 		i=$(($i+1))
>> 	done
>
> /me steps aside to let the shell-script wizards finish the job
I've already pushed out a rewritten one.  Thanks.

The bug makes 1.5.3.3 a dud, and 1.5.3.4 owes credits to Keith and you for fixing it.

Carl Worth· Oct 3, 2007, 15:50 UTC · re: Johannes Sixt · lore

[PATCH] Add test case for ls-files --with-head

This tests basic functionality and also exercises a bug noticed by Keith Packard, (prune_cache followed by add_index_entry can trigger an attempt to realloc a pointer into the middle of an allocated buffer).

Signed-off-by: Carl Worth <cworth@cworth.org>
---
 t/t3060-ls-files-with-head.sh |   55 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)
 create mode 100755 t/t3060-ls-files-with-head.sh
 On Wed, 03 Oct 2007 14:09:13 +0200, Johannes Sixt wrote:
 > seq is not universally available. Can we have that as
 Simple enough. I've included the amended patch, (and I even
 remembered to do the sign-off thing this time).
 Thanks,
 -Carl
Show changes to t/t3060-ls-files-with-head.sh +55 −0
diff --git a/t/t3060-ls-files-with-head.sh b/t/t3060-ls-files-with-head.sh
new file mode 100755
index 0000000..bc3ef58
--- /dev/null
+++ b/t/t3060-ls-files-with-head.sh
@@ -0,0 +1,55 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Carl D. Worth
+#
+
+test_description='gt ls-files test (--with-head).
+
+This test runs git ls-files --with-head and in particular in
+a scenario known to trigger a crash with some versions of git.
+'
+. ./test-lib.sh
+
+# The bug we're exercising requires a fair number of entries in a
+# sub-directory so that add_index_entry will trigger a realloc
+echo file > expected
+mkdir sub
+for i in 0 1 2 3 4; do
+	for j in 0 1 2 3 4 5 6 7 8 9; do
+		> sub/file-$i$j
+		echo file-$i$j >> expected
+	done
+done
+git add .
+git commit -m "add a bunch of files"
+
+# We remove them all so that we'll have something to add back with
+# --with-head and so that we'll definitely be under the realloc size
+# to trigger the bug.
+rm -r sub
+git commit -a -m "remove them all"
+
+# The bug also requires some entry before our directory so that
+# prune_path will modify the_index.cache
+mkdir a_directory_that_sorts_before_sub
+touch a_directory_that_sorts_before_sub/file
+mkdir sub
+touch sub/file
+git add .
+
+# We have to run from a sub-directory to trigger prune_path
+cd sub
+
+# Then we finally get to run our --with-tree test
+test_expect_success \
+    'git -ls-files --with-tree should succeed.' \
+    'git ls-files --with-tree=HEAD~1 >../output'
+
+cd ..
+test_expect_success \
+    'git -ls-files --with-tree should add entries from named tree.' \
+    'diff output expected'
+
+test_done
+
+

-- 
1.5.3.3.131.g34c6d

← back to recent threads