{"thread":{"id":"10125","subject":"[PATCH] Must not modify the_index.cache as it may be passed to realloc at some point.","startedAt":"2007-10-03T05:44:15Z","lastAt":"2007-10-03T22:11:55Z","messageCount":14,"participants":["Keith Packard","Junio C Hamano","Carl Worth","Johannes Sixt","Johannes Schindelin","David Kastrup","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"54661","messageId":"1191390255.16292.2.camel@koto.keithp.com","threadId":"10125","inReplyTo":null,"subject":"[PATCH] Must not modify the_index.cache as it may be passed to realloc at some point.","fromName":"Keith Packard","fromEmail":"keithp@keithp.com","sentAt":"2007-10-03T05:44:15Z","receivedAt":"2007-10-03T05:44:15Z","isPatch":true,"sender":{"key":"keithp@keithp.com","avatar":"https://gravatar.com/avatar/fa1f479cdd51322fe86215c955a81d296bbf66a1fe625f8a12d87a8ec7faf648?d=mp&s=160"},"body":"The index cache is not static, growing as new entries are added. If entries\nare added after prune_cache is called, cache will no longer point at the\nbase of the allocation, and realloc will not be happy.\n\nI verified that this was the only place in the current source which modified\nany index_state.cache elements aside from the alloc/realloc calls in read-cache by\nchanging the type of the element to 'struct cache_entry ** const cache' and\nrecompiling.\n\nA more efficient patch would create a separate 'cache_base' value to track\nthe allocation and then fix things up when reallocation was necessary,\ninstead of the brute-force memmove used here.\n---\n builtin-ls-files.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex 6c1db86..0028b8a 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -280,7 +280,7 @@ static void prune_cache(const char *prefix)\n \n        if (pos < 0)\n                pos = -pos-1;\n-       active_cache += pos;\n+       memmove (active_cache, active_cache + pos, (active_nr - pos) * sizeof (struct cache_entry *));\n        active_nr -= pos;\n        first = 0;\n        last = active_nr;\n-- \n1.5.3.3.131.g34c6d-dirty\n\n-- \nkeith.packard@intel.com\n"},{"id":"54663","messageId":"7vtzp8g2s2.fsf@gitster.siamese.dyndns.org","threadId":"10125","inReplyTo":"1191390255.16292.2.camel@koto.keithp.com","subject":"Re: [PATCH] Must not modify the_index.cache as it may be passed to realloc at some point.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-10-03T05:55:57Z","receivedAt":"2007-10-03T05:55:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Packard <keithp@keithp.com> writes:\n\n> The index cache is not static, growing as new entries are added. If entries\n> are added after prune_cache is called, cache will no longer point at the\n> base of the allocation, and realloc will not be happy.\n\nThanks for catching this.  This code originally was perfectly\nOk, but I broke it with the overlay_tree() change.\n"},{"id":"54669","messageId":"87y7ekr86e.wl%cworth@cworth.org","threadId":"10125","inReplyTo":"7vtzp8g2s2.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Add test case for ls-files --with-head","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2007-10-03T07:03:53Z","receivedAt":"2007-10-03T07:03:53Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"This tests basic functionality and also exercises a bug noticed\nby Keith Packard, (prune_cache followed by add_index_entry can\ntrigger an attempt to realloc a pointer into the middle of an\nallocated buffer).\n---\n t/t3060-ls-files-with-head.sh |   53 +++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 53 insertions(+), 0 deletions(-)\n create mode 100755 t/t3060-ls-files-with-head.sh\n\n On Tue, 02 Oct 2007 22:55:57 -0700, Junio C Hamano wrote:\n >\n > Thanks for catching this.  This code originally was perfectly\n > Ok, but I broke it with the overlay_tree() change.\n\n Yeah, Keith and I were really scratching our heads as to how this\n hadn't caused more problems earlier. I wrote the test case below to\n explore the issue and found the recent overlay_tree change as you\n mention, and that made more sense.\n\n I didn't notice any existing --with-tree test case, so perhaps the\n patch below is useful.\n\n -Carl\n\ndiff --git a/t/t3060-ls-files-with-head.sh b/t/t3060-ls-files-with-head.sh\nnew file mode 100755\nindex 0000000..4ead08b\n--- /dev/null\n+++ b/t/t3060-ls-files-with-head.sh\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2007 Carl D. Worth\n+#\n+\n+test_description='git ls-files test (--with-head).\n+\n+This test runs git ls-files --with-head and in particular in\n+a scenario known to trigger a crash with some versions of git.\n+'\n+. ./test-lib.sh\n+\n+# The bug we're exercising requires a fair number of entries in a\n+# sub-directory so that add_index_entry will trigger a realloc\n+echo file > expected\n+mkdir sub\n+for num in $(seq -f%04g 1 50); do\n+\ttouch sub/file-$num\n+\techo file-$num >> expected\n+done\n+git add .\n+git commit -m \"add a bunch of files\"\n+\n+# We remove them all so that we'll have something to add back with\n+# --with-head and so that we'll definitely be under the realloc size\n+# to trigger the bug.\n+rm -r sub\n+git commit -a -m \"remove them all\"\n+\n+# The bug also requires some entry before our directory so that\n+# prune_path will modify the_index.cache\n+mkdir a_directory_that_sorts_before_sub\n+touch a_directory_that_sorts_before_sub/file\n+mkdir sub\n+touch sub/file\n+git add .\n+\n+# We have to run from a sub-directory to trigger prune_path\n+cd sub\n+\n+# Then we finally get to run our --with-tree test\n+test_expect_success \\\n+    'git -ls-files --with-tree should succeed.' \\\n+    'git ls-files --with-tree=HEAD~1 >../output'\n+\n+cd ..\n+test_expect_success \\\n+    'git -ls-files --with-tree should add entries from named tree.' \\\n+    'diff output expected'\n+\n+test_done\n+\n+\n--\n1.5.3.3.131.g34c6d\n\n"},{"id":"54701","messageId":"47038669.30302@viscovery.net","threadId":"10125","inReplyTo":"87y7ekr86e.wl%cworth@cworth.org","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-10-03T12:09:13Z","receivedAt":"2007-10-03T12:09:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Carl Worth schrieb:\n> +for num in $(seq -f%04g 1 50); do\n> +\ttouch sub/file-$num\n> +\techo file-$num >> expected\n> +done\n\nseq is not universally available. Can we have that as\n\nfor i in 0 1 2 3 4; do\n\tfor j in 0 1 2 3 4 5 6 7 8 9; do\n\t\t> sub/file-$i$j\n\t\techo file-$i$j >> expected\n\tdone\ndone\n\n-- Hannes\n"},{"id":"54715","messageId":"Pine.LNX.4.64.0710031634300.28395@racer.site","threadId":"10125","inReplyTo":"47038669.30302@viscovery.net","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-03T15:36:13Z","receivedAt":"2007-10-03T15:36:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 3 Oct 2007, Johannes Sixt wrote:\n\n> Carl Worth schrieb:\n> > +for num in $(seq -f%04g 1 50); do\n> > +\ttouch sub/file-$num\n> > +\techo file-$num >> expected\n> > +done\n> \n> seq is not universally available. Can we have that as\n> \n> for i in 0 1 2 3 4; do\n> \tfor j in 0 1 2 3 4 5 6 7 8 9; do\n> \t\t> sub/file-$i$j\n> \t\techo file-$i$j >> expected\n> \tdone\n> done\n\nOr as\n\n\ti=1\n\twhile test $i -le 50\n\tdo\n\t\tnum=$(printf %04d $i)\n\t\t> sub/file-$num\n\t\techo file-$num >> expected\n\t\ti=$(($i+1))\n\tdone\n\nThis version should be as portable, with the benefit that it is easier to \nchange for different start and end values.\n\nCiao,\nDscho\n"},{"id":"54716","messageId":"87odfgqjsp.wl%cworth@cworth.org","threadId":"10125","inReplyTo":"47038669.30302@viscovery.net","subject":"[PATCH] Add test case for ls-files --with-head","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2007-10-03T15:50:30Z","receivedAt":"2007-10-03T15:50:30Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"This tests basic functionality and also exercises a bug noticed\nby Keith Packard, (prune_cache followed by add_index_entry can\ntrigger an attempt to realloc a pointer into the middle of an\nallocated buffer).\n\nSigned-off-by: Carl Worth <cworth@cworth.org>\n---\n t/t3060-ls-files-with-head.sh |   55 +++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 55 insertions(+), 0 deletions(-)\n create mode 100755 t/t3060-ls-files-with-head.sh\n\n On Wed, 03 Oct 2007 14:09:13 +0200, Johannes Sixt wrote:\n > seq is not universally available. Can we have that as\n\n Simple enough. I've included the amended patch, (and I even\n remembered to do the sign-off thing this time).\n\n Thanks,\n\n -Carl\n\ndiff --git a/t/t3060-ls-files-with-head.sh b/t/t3060-ls-files-with-head.sh\nnew file mode 100755\nindex 0000000..bc3ef58\n--- /dev/null\n+++ b/t/t3060-ls-files-with-head.sh\n@@ -0,0 +1,55 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2007 Carl D. Worth\n+#\n+\n+test_description='gt ls-files test (--with-head).\n+\n+This test runs git ls-files --with-head and in particular in\n+a scenario known to trigger a crash with some versions of git.\n+'\n+. ./test-lib.sh\n+\n+# The bug we're exercising requires a fair number of entries in a\n+# sub-directory so that add_index_entry will trigger a realloc\n+echo file > expected\n+mkdir sub\n+for i in 0 1 2 3 4; do\n+\tfor j in 0 1 2 3 4 5 6 7 8 9; do\n+\t\t> sub/file-$i$j\n+\t\techo file-$i$j >> expected\n+\tdone\n+done\n+git add .\n+git commit -m \"add a bunch of files\"\n+\n+# We remove them all so that we'll have something to add back with\n+# --with-head and so that we'll definitely be under the realloc size\n+# to trigger the bug.\n+rm -r sub\n+git commit -a -m \"remove them all\"\n+\n+# The bug also requires some entry before our directory so that\n+# prune_path will modify the_index.cache\n+mkdir a_directory_that_sorts_before_sub\n+touch a_directory_that_sorts_before_sub/file\n+mkdir sub\n+touch sub/file\n+git add .\n+\n+# We have to run from a sub-directory to trigger prune_path\n+cd sub\n+\n+# Then we finally get to run our --with-tree test\n+test_expect_success \\\n+    'git -ls-files --with-tree should succeed.' \\\n+    'git ls-files --with-tree=HEAD~1 >../output'\n+\n+cd ..\n+test_expect_success \\\n+    'git -ls-files --with-tree should add entries from named tree.' \\\n+    'diff output expected'\n+\n+test_done\n+\n+\n-- \n1.5.3.3.131.g34c6d\n"},{"id":"54737","messageId":"85odfgry9b.fsf@lola.goethe.zz","threadId":"10125","inReplyTo":"Pine.LNX.4.64.0710031634300.28395@racer.site","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-10-03T15:52:48Z","receivedAt":"2007-10-03T15:52:48Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Wed, 3 Oct 2007, Johannes Sixt wrote:\n>> \n>> seq is not universally available. Can we have that as\n>> \n>> for i in 0 1 2 3 4; do\n>> \tfor j in 0 1 2 3 4 5 6 7 8 9; do\n>> \t\t> sub/file-$i$j\n>> \t\techo file-$i$j >> expected\n>> \tdone\n>> done\n>\n> Or as\n>\n> \ti=1\n> \twhile test $i -le 50\n> \tdo\n> \t\tnum=$(printf %04d $i)\n> \t\t> sub/file-$num\n> \t\techo file-$num >> expected\n> \t\ti=$(($i+1))\n> \tdone\n>\n> This version should be as portable,\n\nHuh?  It uses the conceivably-not-builtin \"test\" (something which\n_you_ picked as something to complain about in a patch of mine where\nit was not used in an inner loop) on every iteration, it uses printf\nand it uses $((...))  arithmetic expansion.  Whereas the proposal by\nJohannes works fine even on prehistoric shell versions.  So the \"as\nportable\" enough moniker is surely weird.\n\n> with the benefit that it is easier to change for different start and\n> end values.\n\nCorrect.  But why would we want those here?\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"54720","messageId":"87myv0qj2u.wl%cworth@cworth.org","threadId":"10125","inReplyTo":"Pine.LNX.4.64.0710031634300.28395@racer.site","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2007-10-03T16:06:01Z","receivedAt":"2007-10-03T16:06:01Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Wed, 3 Oct 2007 16:36:13 +0100 (BST), Johannes Schindelin wrote:\n> Or as\n>\n> \ti=1\n> \twhile test $i -le 50\n> \tdo\n...\n> \t\ti=$(($i+1))\n> \tdone\n\n/me steps aside to let the shell-script wizards finish the job\n\n-Carl\n"},{"id":"54738","messageId":"85ejgcrx6r.fsf@lola.goethe.zz","threadId":"10125","inReplyTo":"87myv0qj2u.wl%cworth@cworth.org","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-10-03T16:15:56Z","receivedAt":"2007-10-03T16:15:56Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Carl Worth <cworth@cworth.org> writes:\n\n> On Wed, 3 Oct 2007 16:36:13 +0100 (BST), Johannes Schindelin wrote:\n>> Or as\n>>\n>> \ti=1\n>> \twhile test $i -le 50\n>> \tdo\n> ...\n>> \t\ti=$(($i+1))\n>> \tdone\n>\n> /me steps aside to let the shell-script wizards finish the job\n\nfor i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}\ndo\n  ...\ndone\n\nThere is enough room for perversion in shell programming for everyone...\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"54740","messageId":"7vlkakdmjf.fsf@gitster.siamese.dyndns.org","threadId":"10125","inReplyTo":"87myv0qj2u.wl%cworth@cworth.org","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-10-03T19:29:40Z","receivedAt":"2007-10-03T19:29:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carl Worth <cworth@cworth.org> writes:\n\n> On Wed, 3 Oct 2007 16:36:13 +0100 (BST), Johannes Schindelin wrote:\n>> Or as\n>>\n>> \ti=1\n>> \twhile test $i -le 50\n>> \tdo\n> ...\n>> \t\ti=$(($i+1))\n>> \tdone\n>\n> /me steps aside to let the shell-script wizards finish the job\n\nI've already pushed out a rewritten one.  Thanks.\n\nThe bug makes 1.5.3.3 a dud, and 1.5.3.4 owes credits to Keith\nand you for fixing it.\n"},{"id":"54750","messageId":"20071003202157.GA28043@coredump.intra.peff.net","threadId":"10125","inReplyTo":"85ejgcrx6r.fsf@lola.goethe.zz","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-10-03T20:21:58Z","receivedAt":"2007-10-03T20:21:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 03, 2007 at 06:15:56PM +0200, David Kastrup wrote:\n\n> for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}\n> do\n>   ...\n> done\n\n$ dash\n$ for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}; do echo $i; done\n{1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}\n$\n\n-Peff\n"},{"id":"54762","messageId":"Pine.LNX.4.64.0710032238080.28395@racer.site","threadId":"10125","inReplyTo":"20071003202157.GA28043@coredump.intra.peff.net","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-03T21:39:48Z","receivedAt":"2007-10-03T21:39:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 3 Oct 2007, Jeff King wrote:\n\n> > for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}\n> > do\n> >   ...\n> > done\n> \n> $ dash\n> $ for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}; do echo $i; done\n> {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}\n> $\n\nAFAIK this is the same as bash (I thought I was the last one to make that \nmistake 10 years ago).  As long as you do not have _files_ matching the \npattern, it does not expand.  And besides, this is too complicated anyway: \n[1-5] is much shorter than {1,2,3,4,5}.\n\nCiao,\nDscho\n"},{"id":"54767","messageId":"7vodffdg6i.fsf@gitster.siamese.dyndns.org","threadId":"10125","inReplyTo":"Pine.LNX.4.64.0710032238080.28395@racer.site","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-10-03T21:47:01Z","receivedAt":"2007-10-03T21:47:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> $ for i in {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}; do echo $i; done\n>> {1,2,3,4,5}{0,1,2,3,4,5,6,7,8,9}\n>> $\n>\n> AFAIK this is the same as bash (I thought I was the last one to make that \n> mistake 10 years ago).  As long as you do not have _files_ matching the \n> pattern, it does not expand.  And besides, this is too complicated anyway: \n> [1-5] is much shorter than {1,2,3,4,5}.\n\nAFAIK, you are wrong ;-)\n\n{1,2,3,4,5} expands regardless of what's on the filesystem but I\ndo not think it is POSIX.\n\n[1-5] matches if any of the {1,2,3,4,5} is found on the\nfilesystem.\n"},{"id":"54770","messageId":"20071003221155.GA28491@coredump.intra.peff.net","threadId":"10125","inReplyTo":"7vodffdg6i.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add test case for ls-files --with-head","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-10-03T22:11:55Z","receivedAt":"2007-10-03T22:11:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 03, 2007 at 02:47:01PM -0700, Junio C Hamano wrote:\n\n> AFAIK, you are wrong ;-)\n> \n> {1,2,3,4,5} expands regardless of what's on the filesystem but I\n> do not think it is POSIX.\n\nYes, I think that is right.\n\n-Peff\n"}]}