{"thread":{"id":"34428","subject":"Segfault in `git describe`","startedAt":"2013-07-13T13:27:33Z","lastAt":"2013-07-24T14:35:24Z","messageCount":8,"participants":["Mantas Mikulėnas","Michael Haggerty","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"223245","messageId":"krrkk0$kri$1@ger.gmane.org","threadId":"34428","inReplyTo":null,"subject":"Segfault in `git describe`","fromName":"Mantas Mikulėnas","fromEmail":"grawity@gmail.com","sentAt":"2013-07-13T13:27:33Z","receivedAt":"2013-07-13T13:27:33Z","isPatch":false,"sender":{"key":"grawity@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31021?v=4"},"body":"I have a clone of linux.git with various stuff added to it (remotes for\n'stable' and 'next', a bunch of local tags, and historical repositories\nimported using `git replace`).\n\nYesterday, I noticed that `git describe`, built from git.git master\n(v1.8.3.2-804-g0da7a53, gcc 4.8) would simply crash when run in that\nrepository, with the following backtrace:\n\n> Program terminated with signal 11, Segmentation fault.\n> #0 0x00000000004c39dc in hashcpy (sha_src=0x1c
, \n> sha_dst=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\")\n> at cache.h:694\n> 694\t\tmemcpy(sha_dst, sha_src, 20);\n> (gdb) bt\n> #0 0x00000000004c39dc in hashcpy (sha_src=0x1c
, \n> sha_dst=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\")\n> at cache.h:694\n> #1 peel_ref (refname=refname@entry=0x1fe2d10 \"refs/tags/next-20130607\", \n> sha1=sha1@entry=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\") at refs.c:1586\n> #2 0x0000000000424194 in get_name (path=0x1fe2d10 \"refs/tags/next-20130607\", \n> sha1=0x1fe2ce8 \"\\222V\\356\\276S5\\tk\\231Hi\\264\\r=\\336\\315\\302\\225\\347\\257\\300N\\376\\327\\064@\\237ZDq[T\\246\\312\\033T\\260\\314\\362\\025refs/tags/next-20130607\", flag=, \n> cb_data=) at builtin/describe.c:156\n> #3 0x00000000004c1c21 in do_one_ref (entry=0x1fe2ce0, cb_data=0x7fffc0b4d7c0)\n> at refs.c:646\n> #4 0x00000000004c318d in do_for_each_entry_in_dir (dir=0x1fe1728, \n> offset=, fn=0x4c1bc0 , cb_data=0x7fffc0b4d7c0)\n> at refs.c:672\n> #5 0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf4d8, dir2=0x1fd6318, \n> cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 ) at refs.c:716\n> #6 0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf1f8, dir2=0x1fd62d8, \n> cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 ) at refs.c:716\n> #7 0x00000000004c3540 in do_for_each_entry (refs=refs@entry=0x7a2800 , \n> base=base@entry=0x509cc6 \"\", cb_data=cb_data@entry=0x7fffc0b4d7c0, \n> fn=0x4c1bc0 ) at refs.c:1689\n> #8 0x00000000004c3ff8 in do_for_each_ref (cb_data=cb_data@entry=0x0, flags=1, trim=0, \n> fn=fn@entry=0x424120 , base=0x509cc6 \"\", refs=0x7a2800 )\n> at refs.c:1724\n> #9 for_each_rawref (fn=fn@entry=0x424120 , cb_data=cb_data@entry=0x0)\n> at refs.c:1873\n> #10 0x0000000000424f5b in cmd_describe (argc=0, argv=0x7fffc0b4ddc0, prefix=0x0)\n> at builtin/describe.c:466\n> #11 0x000000000040596d in run_builtin (argv=0x7fffc0b4ddc0, argc=1, \n> p=0x760b40 ) at git.c:291\n> #12 handle_internal_command (argc=1, argv=0x7fffc0b4ddc0) at git.c:453\n> #13 0x0000000000404d6e in run_argv (argv=0x7fffc0b4dc78, argcp=0x7fffc0b4dc5c)\n> at git.c:499\n> #14 main (argc=1, av=) at git.c:575\n> (gdb) \n\nAccording to `git bisect`, the first bad commit is:\n\ncommit 9a489f3c17d6c974b18c47cf406404ca2a721c87\nAuthor: Michael Haggerty \nDate: Mon Apr 22 21:52:22 2013 +0200\n\n refs: extract a function peel_entry()\n\nThe crash happens only in repositories that have at least one replaced\nobject in the branch's history. Running `git --no-replace-objects\ndescribe` avoids the crash.\n\nThe crash happens only if there are any tags under .git/refs/tags/ that\ndo not exist in .git/packed-refs, or if I remove all \"peeled\" lines from\n.git/packed-refs (including the '#' line; /^[#^]/d).\n\nA quick way to reproduce this with git.git master is:\n\ngit tag -f test-tag HEAD~10\ngit replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \\\n | sed 's/@/@test/' | git hash-object --stdin -t commit -w)\n./git describe\n\n-- \nMantas Mikulėnas \n"},{"id":"223421","messageId":"51E3F337.8070708@alum.mit.edu","threadId":"34428","inReplyTo":"krrkk0$kri$1@ger.gmane.org","subject":"Re: Segfault in `git describe`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-15T13:03:51Z","receivedAt":"2013-07-15T13:03:51Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/13/2013 03:27 PM, Mantas Mikulėnas wrote:\n> I have a clone of linux.git with various stuff added to it (remotes for\n> 'stable' and 'next', a bunch of local tags, and historical repositories\n> imported using `git replace`).\n> \n> Yesterday, I noticed that `git describe`, built from git.git master\n> (v1.8.3.2-804-g0da7a53, gcc 4.8) would simply crash when run in that\n> repository, with the following backtrace:\n> \n>> Program terminated with signal 11, Segmentation fault.\n>> #0 0x00000000004c39dc in hashcpy (sha_src=0x1c
, \n>> sha_dst=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\")\n>> at cache.h:694\n>> 694\t\tmemcpy(sha_dst, sha_src, 20);\n>> (gdb) bt\n>> #0 0x00000000004c39dc in hashcpy (sha_src=0x1c
, \n>> sha_dst=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\")\n>> at cache.h:694\n>> #1 peel_ref (refname=refname@entry=0x1fe2d10 \"refs/tags/next-20130607\", \n>> sha1=sha1@entry=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\") at refs.c:1586\n>> #2 0x0000000000424194 in get_name (path=0x1fe2d10 \"refs/tags/next-20130607\", \n>> sha1=0x1fe2ce8 \"\\222V\\356\\276S5\\tk\\231Hi\\264\\r=\\336\\315\\302\\225\\347\\257\\300N\\376\\327\\064@\\237ZDq[T\\246\\312\\033T\\260\\314\\362\\025refs/tags/next-20130607\", flag=, \n>> cb_data=) at builtin/describe.c:156\n>> #3 0x00000000004c1c21 in do_one_ref (entry=0x1fe2ce0, cb_data=0x7fffc0b4d7c0)\n>> at refs.c:646\n>> #4 0x00000000004c318d in do_for_each_entry_in_dir (dir=0x1fe1728, \n>> offset=, fn=0x4c1bc0 , cb_data=0x7fffc0b4d7c0)\n>> at refs.c:672\n>> #5 0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf4d8, dir2=0x1fd6318, \n>> cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 ) at refs.c:716\n>> #6 0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf1f8, dir2=0x1fd62d8, \n>> cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 ) at refs.c:716\n>> #7 0x00000000004c3540 in do_for_each_entry (refs=refs@entry=0x7a2800 , \n>> base=base@entry=0x509cc6 \"\", cb_data=cb_data@entry=0x7fffc0b4d7c0, \n>> fn=0x4c1bc0 ) at refs.c:1689\n>> #8 0x00000000004c3ff8 in do_for_each_ref (cb_data=cb_data@entry=0x0, flags=1, trim=0, \n>> fn=fn@entry=0x424120 , base=0x509cc6 \"\", refs=0x7a2800 )\n>> at refs.c:1724\n>> #9 for_each_rawref (fn=fn@entry=0x424120 , cb_data=cb_data@entry=0x0)\n>> at refs.c:1873\n>> #10 0x0000000000424f5b in cmd_describe (argc=0, argv=0x7fffc0b4ddc0, prefix=0x0)\n>> at builtin/describe.c:466\n>> #11 0x000000000040596d in run_builtin (argv=0x7fffc0b4ddc0, argc=1, \n>> p=0x760b40 ) at git.c:291\n>> #12 handle_internal_command (argc=1, argv=0x7fffc0b4ddc0) at git.c:453\n>> #13 0x0000000000404d6e in run_argv (argv=0x7fffc0b4dc78, argcp=0x7fffc0b4dc5c)\n>> at git.c:499\n>> #14 main (argc=1, av=) at git.c:575\n>> (gdb) \n> \n> According to `git bisect`, the first bad commit is:\n> \n> commit 9a489f3c17d6c974b18c47cf406404ca2a721c87\n> Author: Michael Haggerty \n> Date: Mon Apr 22 21:52:22 2013 +0200\n> \n> refs: extract a function peel_entry()\n> \n> The crash happens only in repositories that have at least one replaced\n> object in the branch's history. Running `git --no-replace-objects\n> describe` avoids the crash.\n> \n> The crash happens only if there are any tags under .git/refs/tags/ that\n> do not exist in .git/packed-refs, or if I remove all \"peeled\" lines from\n> .git/packed-refs (including the '#' line; /^[#^]/d).\n> \n> A quick way to reproduce this with git.git master is:\n> \n> git tag -f test-tag HEAD~10\n> git replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \\\n> | sed 's/@/@test/' | git hash-object --stdin -t commit -w)\n> ./git describe\n\nThanks for the bug report.\n\nI think the cause of this bug is that peel_entry() is causing a nested\ncall to do_for_each_entry() to look up the replace reference, which\nresets current_ref to NULL between the test and the dereference of\ncurrent_ref in peel_ref().\n\nUnfortunately, I cannot reproduce the failure by following your recipe\n(though I didn't have a lot of time yet for this). I suppose that my\nrepo starts out in a slightly different state than yours and therefore I\ndon't get the same results. If you could find a recipe to reproduce the\nproblem, starting either with an empty repo, or perhaps a fresh clone of\ngit.git, and double-check that you don't have any unusual config options\nthat might be affecting things, that would be very helpful.\n\nI might have more time to look at this tonight.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"223428","messageId":"CAPWNY8Ua=3t4jeDvkj3Aw2Ouvv+0r1kWrET5GNq9uS8PasGudQ@mail.gmail.com","threadId":"34428","inReplyTo":"51E3F337.8070708@alum.mit.edu","subject":"Re: Segfault in `git describe`","fromName":"Mantas Mikulėnas","fromEmail":"grawity@gmail.com","sentAt":"2013-07-15T13:31:38Z","receivedAt":"2013-07-15T13:31:38Z","isPatch":false,"sender":{"key":"grawity@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31021?v=4"},"body":"On Mon, Jul 15, 2013 at 4:03 PM, Michael Haggerty wrote:\n> On 07/13/2013 03:27 PM, Mantas Mikulėnas wrote:\n>> I have a clone of linux.git with various stuff added to it (remotes for\n>> 'stable' and 'next', a bunch of local tags, and historical repositories\n>> imported using `git replace`).\n>>\n>> Yesterday, I noticed that `git describe`, built from git.git master\n>> (v1.8.3.2-804-g0da7a53, gcc 4.8) would simply crash when run in that\n>> repository, with the following backtrace:\n>>\n>>> Program terminated with signal 11, Segmentation fault.\n>>> #0 0x00000000004c39dc in hashcpy (sha_src=0x1c
,\n>>> sha_dst=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\")\n>>> at cache.h:694\n>>> 694 memcpy(sha_dst, sha_src, 20);\n>>> (gdb) bt\n>>> #0 0x00000000004c39dc in hashcpy (sha_src=0x1c
,\n>>> sha_dst=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\")\n>>> at cache.h:694\n>>> #1 peel_ref (refname=refname@entry=0x1fe2d10 \"refs/tags/next-20130607\",\n>>> sha1=sha1@entry=0x7fffc0b4d610 \"\\242\\271\\301\\366 \\201&\\346\\337l\\002B\\214P\\037\\210ShX\\022\") at refs.c:1586\n>>> #2 0x0000000000424194 in get_name (path=0x1fe2d10 \"refs/tags/next-20130607\",\n>>> sha1=0x1fe2ce8 \"\\222V\\356\\276S5\\tk\\231Hi\\264\\r=\\336\\315\\302\\225\\347\\257\\300N\\376\\327\\064@\\237ZDq[T\\246\\312\\033T\\260\\314\\362\\025refs/tags/next-20130607\", flag=,\n>>> cb_data=) at builtin/describe.c:156\n>>> #3 0x00000000004c1c21 in do_one_ref (entry=0x1fe2ce0, cb_data=0x7fffc0b4d7c0)\n>>> at refs.c:646\n>>> #4 0x00000000004c318d in do_for_each_entry_in_dir (dir=0x1fe1728,\n>>> offset=, fn=0x4c1bc0 , cb_data=0x7fffc0b4d7c0)\n>>> at refs.c:672\n>>> #5 0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf4d8, dir2=0x1fd6318,\n>>> cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 ) at refs.c:716\n>>> #6 0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf1f8, dir2=0x1fd62d8,\n>>> cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 ) at refs.c:716\n>>> #7 0x00000000004c3540 in do_for_each_entry (refs=refs@entry=0x7a2800 ,\n>>> base=base@entry=0x509cc6 \"\", cb_data=cb_data@entry=0x7fffc0b4d7c0,\n>>> fn=0x4c1bc0 ) at refs.c:1689\n>>> #8 0x00000000004c3ff8 in do_for_each_ref (cb_data=cb_data@entry=0x0, flags=1, trim=0,\n>>> fn=fn@entry=0x424120 , base=0x509cc6 \"\", refs=0x7a2800 )\n>>> at refs.c:1724\n>>> #9 for_each_rawref (fn=fn@entry=0x424120 , cb_data=cb_data@entry=0x0)\n>>> at refs.c:1873\n>>> #10 0x0000000000424f5b in cmd_describe (argc=0, argv=0x7fffc0b4ddc0, prefix=0x0)\n>>> at builtin/describe.c:466\n>>> #11 0x000000000040596d in run_builtin (argv=0x7fffc0b4ddc0, argc=1,\n>>> p=0x760b40 ) at git.c:291\n>>> #12 handle_internal_command (argc=1, argv=0x7fffc0b4ddc0) at git.c:453\n>>> #13 0x0000000000404d6e in run_argv (argv=0x7fffc0b4dc78, argcp=0x7fffc0b4dc5c)\n>>> at git.c:499\n>>> #14 main (argc=1, av=) at git.c:575\n>>> (gdb)\n>>\n>> According to `git bisect`, the first bad commit is:\n>>\n>> commit 9a489f3c17d6c974b18c47cf406404ca2a721c87\n>> Author: Michael Haggerty \n>> Date: Mon Apr 22 21:52:22 2013 +0200\n>>\n>> refs: extract a function peel_entry()\n>>\n>> The crash happens only in repositories that have at least one replaced\n>> object in the branch's history. Running `git --no-replace-objects\n>> describe` avoids the crash.\n>>\n>> The crash happens only if there are any tags under .git/refs/tags/ that\n>> do not exist in .git/packed-refs, or if I remove all \"peeled\" lines from\n>> .git/packed-refs (including the '#' line; /^[#^]/d).\n>>\n>> A quick way to reproduce this with git.git master is:\n>>\n>> git tag -f test-tag HEAD~10\n>> git replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \\\n>> | sed 's/@/@test/' | git hash-object --stdin -t commit -w)\n>> ./git describe\n>\n> Thanks for the bug report.\n>\n> I think the cause of this bug is that peel_entry() is causing a nested\n> call to do_for_each_entry() to look up the replace reference, which\n> resets current_ref to NULL between the test and the dereference of\n> current_ref in peel_ref().\n>\n> Unfortunately, I cannot reproduce the failure by following your recipe\n> (though I didn't have a lot of time yet for this). I suppose that my\n> repo starts out in a slightly different state than yours and therefore I\n> don't get the same results. If you could find a recipe to reproduce the\n> problem, starting either with an empty repo, or perhaps a fresh clone of\n> git.git, and double-check that you don't have any unusual config options\n> that might be affecting things, that would be very helpful.\n\nHmm, yes, just creating a new tag doesn't break in another\nfreshly-cloned repo, either.\n\nHowever,\n\n> …or if I remove all \"peeled\" lines from .git/packed-refs (including the '#' line; /^[#^]/d).\n\nstill works for reproducing the crash. When packed-refs does not have\nany peeled refs, older git versions do it manually (I assume for\ncompatibility with even older git versions), while the latest one\ncrashes. This recipe should work:\n\ngit pack-refs --all --prune\nsed -i '/^[#^]/d' .git/packed-refs\ngit replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \\\n | sed 's/@/@test/' | git hash-object --stdin -t commit -w)\n~/src/git/git describe\n\n--\nMantas Mikulėnas \n"},{"id":"223438","messageId":"1373901857-28431-1-git-send-email-mhagger@alum.mit.edu","threadId":"34428","inReplyTo":"CAPWNY8Ua=3t4jeDvkj3Aw2Ouvv+0r1kWrET5GNq9uS8PasGudQ@mail.gmail.com","subject":"[PATCH] do_one_ref(): save and restore value of current_ref","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-15T15:24:17Z","receivedAt":"2013-07-15T15:24:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If do_one_ref() is called recursively, then the inner call should not\npermanently overwrite the value stored in current_ref by the outer\ncall. Aside from the tiny optimization loss, peel_ref() expects the\nvalue of current_ref not to change across a call to peel_entry(). But\nin the presence of replace references that assumption could be\nviolated by a recursive call to do_one_ref:\n\ndo_for_each_entry()\n do_one_ref()\n builtin/describe.c:get_name()\n peel_ref()\n peel_entry()\n peel_object ()\n deref_tag_noverify()\n parse_object()\n lookup_replace_object()\n do_lookup_replace_object()\n prepare_replace_object()\n do_for_each_ref()\n do_for_each_entry()\n do_for_each_entry_in_dir()\n do_one_ref()\n\nThe inner call to do_one_ref() was unconditionally setting current_ref\nto NULL when it was done, causing peel_ref() to perform an invalid\nmemory access.\n\nSo change do_one_ref() to save the old value of current_ref before\noverwriting it, and restore the old value afterward rather than\nsetting it to NULL.\n\nReported by: Mantas Mikulėnas \n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4302206..222baf2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -634,7 +634,9 @@ struct ref_entry_cb {\n static int do_one_ref(struct ref_entry *entry, void *cb_data)\n {\n \tstruct ref_entry_cb *data = cb_data;\n+\tstruct ref_entry *old_current_ref;\n \tint retval;\n+\n \tif (prefixcmp(entry->name, data->base))\n \t\treturn 0;\n \n@@ -642,10 +644,12 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)\n \t !ref_resolves_to_object(entry))\n \t\treturn 0;\n \n+\t/* Store the old value, in case this is a recursive call: */\n+\told_current_ref = current_ref;\n \tcurrent_ref = entry;\n \tretval = data->fn(entry->name + data->trim, entry->u.value.sha1,\n \t\t\t entry->flag, data->cb_data);\n-\tcurrent_ref = NULL;\n+\tcurrent_ref = old_current_ref;\n \treturn retval;\n }\n \n-- \n1.8.3.2\n"},{"id":"223633","messageId":"7voba04ir8.fsf@alter.siamese.dyndns.org","threadId":"34428","inReplyTo":"1373901857-28431-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH] do_one_ref(): save and restore value of current_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T04:03:23Z","receivedAt":"2013-07-18T04:03:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> If do_one_ref() is called recursively, then the inner call should not\n> permanently overwrite the value stored in current_ref by the outer\n> call. Aside from the tiny optimization loss, peel_ref() expects the\n> value of current_ref not to change across a call to peel_entry(). But\n> in the presence of replace references that assumption could be\n> violated by a recursive call to do_one_ref:\n>\n> do_for_each_entry()\n> do_one_ref()\n> builtin/describe.c:get_name()\n> peel_ref()\n> peel_entry()\n> peel_object ()\n> deref_tag_noverify()\n> parse_object()\n> lookup_replace_object()\n> do_lookup_replace_object()\n> prepare_replace_object()\n> do_for_each_ref()\n> do_for_each_entry()\n> do_for_each_entry_in_dir()\n> do_one_ref()\n>\n> The inner call to do_one_ref() was unconditionally setting current_ref\n> to NULL when it was done, causing peel_ref() to perform an invalid\n> memory access.\n>\n> So change do_one_ref() to save the old value of current_ref before\n> overwriting it, and restore the old value afterward rather than\n> setting it to NULL.\n>\n> Reported by: Mantas Mikulėnas \n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nThanks.\n\ns/Reported by:/Reported-by:/ and lose the extra blank line after it?\n\nI wonder if we can have an easy reproduction recipe in our tests.\n"},{"id":"223788","messageId":"51E97ACA.40300@alum.mit.edu","threadId":"34428","inReplyTo":"7voba04ir8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] do_one_ref(): save and restore value of current_ref","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-19T17:43:38Z","receivedAt":"2013-07-19T17:43:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/17/2013 09:03 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> If do_one_ref() is called recursively, then the inner call should not\n>> permanently overwrite the value stored in current_ref by the outer\n>> call. Aside from the tiny optimization loss, peel_ref() expects the\n>> value of current_ref not to change across a call to peel_entry(). But\n>> in the presence of replace references that assumption could be\n>> violated by a recursive call to do_one_ref:\n>>\n>> do_for_each_entry()\n>> do_one_ref()\n>> builtin/describe.c:get_name()\n>> peel_ref()\n>> peel_entry()\n>> peel_object ()\n>> deref_tag_noverify()\n>> parse_object()\n>> lookup_replace_object()\n>> do_lookup_replace_object()\n>> prepare_replace_object()\n>> do_for_each_ref()\n>> do_for_each_entry()\n>> do_for_each_entry_in_dir()\n>> do_one_ref()\n>>\n>> The inner call to do_one_ref() was unconditionally setting current_ref\n>> to NULL when it was done, causing peel_ref() to perform an invalid\n>> memory access.\n>>\n>> So change do_one_ref() to save the old value of current_ref before\n>> overwriting it, and restore the old value afterward rather than\n>> setting it to NULL.\n>>\n>> Reported by: Mantas Mikulėnas \n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n> \n> Thanks.\n> \n> s/Reported by:/Reported-by:/ and lose the extra blank line after it?\n\nACK, sorry I got the notation wrong.\n\n> I wonder if we can have an easy reproduction recipe in our tests.\n\nI could reproduce the problem by following the recipe provided by Mantas\nupthread (or at least something very close to it; I can't find the\nscript that I was using):\n\n> git pack-refs --all --prune\n> sed -i '/^[#^]/d' .git/packed-refs\n> git replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \\\n> | sed 's/@/@test/' | git hash-object --stdin -t commit -w)\n> ~/src/git/git describe\n\nIt would be good to document this in the commit message, but I don't\nthink it is necessary to have a test for it in the test suite because I\ndon't think it is the kind of bug that will reappear.\n\nI sent the patch shortly before leaving for a trip so I didn't have time\nto make it as complete as I would have liked. But given that the\nproblem was already in master, and the fix is pretty simple, I wanted to\nsend the fix right away. When I have some time I can fix it up better,\nor feel free to manhandle the patch and/or commit message yourself if\nyou prefer.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"223797","messageId":"7vppueuyw4.fsf@alter.siamese.dyndns.org","threadId":"34428","inReplyTo":"51E97ACA.40300@alum.mit.edu","subject":"Re: [PATCH] do_one_ref(): save and restore value of current_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-19T19:34:51Z","receivedAt":"2013-07-19T19:34:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I sent the patch shortly before leaving for a trip so I didn't have time\n> to make it as complete as I would have liked. But given that the\n> problem was already in master, and the fix is pretty simple, I wanted to\n> send the fix right away. When I have some time I can fix it up better,\n\nThat is very much appreciated. How would you describe this fix in a\ntwo-to-three line paragraph in Release Notes?\n"},{"id":"224042","messageId":"51EFE62C.4070003@alum.mit.edu","threadId":"34428","inReplyTo":"7vppueuyw4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] do_one_ref(): save and restore value of current_ref","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-24T14:35:24Z","receivedAt":"2013-07-24T14:35:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/19/2013 12:34 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> I sent the patch shortly before leaving for a trip so I didn't have time\n>> to make it as complete as I would have liked. But given that the\n>> problem was already in master, and the fix is pretty simple, I wanted to\n>> send the fix right away. When I have some time I can fix it up better,\n> \n> That is very much appreciated. How would you describe this fix in a\n> two-to-three line paragraph in Release Notes?\n\nHow about:\n\n Fix a NULL-pointer dereference during nested iterations over\n references (for example, when replace references are being used).\n\nUnfortunately I don't have time now to audit the code more carefully to\nfigure out what other circumstances might have triggered the bug.\n\nHope that helps,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}