# [PATCH] Must not modify the_index.cache as it may be passed to realloc at some point. 14 messages from 2007-10-03 to 2007-10-03. Participants: Keith Packard, Junio C Hamano, Carl Worth, Johannes Sixt, Johannes Schindelin, David Kastrup, Jeff King. Thread: https://gitlist.dev/t/10125 ## Keith Packard, 2007-10-03 05:44 Subject: [PATCH] Must not modify the_index.cache as it may be passed to realloc at some point. Message-ID: <1191390255.16292.2.camel@koto.keithp.com> URL: https://gitlist.dev/e/1191390255.16292.2.camel%40koto.keithp.com ``` 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 ``` ## Junio C Hamano, 2007-10-03 05:55 Subject: Re: [PATCH] Must not modify the_index.cache as it may be passed to realloc at some point. Message-ID: <7vtzp8g2s2.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzp8g2s2.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1191390255.16292.2.camel@koto.keithp.com> ``` Keith Packard 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, 2007-10-03 07:03 Subject: [PATCH] Add test case for ls-files --with-head Message-ID: <87y7ekr86e.wl%cworth@cworth.org> URL: https://gitlist.dev/e/87y7ekr86e.wl%25cworth%40cworth.org In-Reply-To: <7vtzp8g2s2.fsf@gitster.siamese.dyndns.org> ``` 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 ``` ## Johannes Sixt, 2007-10-03 12:09 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: <47038669.30302@viscovery.net> URL: https://gitlist.dev/e/47038669.30302%40viscovery.net In-Reply-To: <87y7ekr86e.wl%cworth@cworth.org> ``` 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, 2007-10-03 15:36 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710031634300.28395%40racer.site In-Reply-To: <47038669.30302@viscovery.net> ``` 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 ``` ## Carl Worth, 2007-10-03 15:50 Subject: [PATCH] Add test case for ls-files --with-head Message-ID: <87odfgqjsp.wl%cworth@cworth.org> URL: https://gitlist.dev/e/87odfgqjsp.wl%25cworth%40cworth.org In-Reply-To: <47038669.30302@viscovery.net> ``` 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 --- 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 ``` ## David Kastrup, 2007-10-03 15:52 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: <85odfgry9b.fsf@lola.goethe.zz> URL: https://gitlist.dev/e/85odfgry9b.fsf%40lola.goethe.zz In-Reply-To: ``` Johannes Schindelin 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 ``` ## Carl Worth, 2007-10-03 16:06 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: <87myv0qj2u.wl%cworth@cworth.org> URL: https://gitlist.dev/e/87myv0qj2u.wl%25cworth%40cworth.org In-Reply-To: ``` 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 ``` ## David Kastrup, 2007-10-03 16:15 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: <85ejgcrx6r.fsf@lola.goethe.zz> URL: https://gitlist.dev/e/85ejgcrx6r.fsf%40lola.goethe.zz In-Reply-To: <87myv0qj2u.wl%cworth@cworth.org> ``` Carl Worth 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 ``` ## Junio C Hamano, 2007-10-03 19:29 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: <7vlkakdmjf.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vlkakdmjf.fsf%40gitster.siamese.dyndns.org In-Reply-To: <87myv0qj2u.wl%cworth@cworth.org> ``` Carl Worth 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. ``` ## Jeff King, 2007-10-03 20:21 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: <20071003202157.GA28043@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071003202157.GA28043%40coredump.intra.peff.net In-Reply-To: <85ejgcrx6r.fsf@lola.goethe.zz> ``` 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, 2007-10-03 21:39 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0710032238080.28395%40racer.site In-Reply-To: <20071003202157.GA28043@coredump.intra.peff.net> ``` 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 ``` ## Junio C Hamano, 2007-10-03 21:47 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: <7vodffdg6i.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vodffdg6i.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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. ``` ## Jeff King, 2007-10-03 22:11 Subject: Re: [PATCH] Add test case for ls-files --with-head Message-ID: <20071003221155.GA28491@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071003221155.GA28491%40coredump.intra.peff.net In-Reply-To: <7vodffdg6i.fsf@gitster.siamese.dyndns.org> ``` 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 ```