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

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

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

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(-)

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Junio C Hamano]

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.

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

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]

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

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


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Johannes Sixt]

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

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

[Johannes Schindelin]

Hi,

On Wed, 3 Oct 2007, Johannes Sixt wrote:

> 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

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

[Carl Worth]

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

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

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

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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

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

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 214 bytes --]

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

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[David Kastrup]

Carl Worth <cworth@cworth.org> writes:

> 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

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

[Junio C Hamano]

Carl Worth <cworth@cworth.org> writes:

> 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.

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

[Jeff King]

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

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

[Johannes Schindelin]

Hi,

On Wed, 3 Oct 2007, Jeff King 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}
> $

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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> $ 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.

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

[Jeff King]

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