All of lore.kernel.org
 help / color / mirror / Atom feed
* Segfault in `git describe`
@ 2013-07-13 13:27 Mantas Mikulėnas
  2013-07-15 13:03 ` Michael Haggerty
  0 siblings, 1 reply; 8+ messages in thread
From: Mantas Mikulėnas @ 2013-07-13 13:27 UTC (permalink / raw)
  To: git

I have a clone of linux.git with various stuff added to it (remotes for
'stable' and 'next', a bunch of local tags, and historical repositories
imported using `git replace`).

Yesterday, I noticed that `git describe`, built from git.git master
(v1.8.3.2-804-g0da7a53, gcc 4.8) would simply crash when run in that
repository, with the following backtrace:

> Program terminated with signal 11, Segmentation fault.
> #0  0x00000000004c39dc in hashcpy (sha_src=0x1c <Address 0x1c out of bounds>, 
>     sha_dst=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022")
>     at cache.h:694
> 694		memcpy(sha_dst, sha_src, 20);
> (gdb) bt
> #0  0x00000000004c39dc in hashcpy (sha_src=0x1c <Address 0x1c out of bounds>, 
>     sha_dst=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022")
>     at cache.h:694
> #1  peel_ref (refname=refname@entry=0x1fe2d10 "refs/tags/next-20130607", 
>     sha1=sha1@entry=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022") at refs.c:1586
> #2  0x0000000000424194 in get_name (path=0x1fe2d10 "refs/tags/next-20130607", 
>     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=<optimized out>, 
>     cb_data=<optimized out>) at builtin/describe.c:156
> #3  0x00000000004c1c21 in do_one_ref (entry=0x1fe2ce0, cb_data=0x7fffc0b4d7c0)
>     at refs.c:646
> #4  0x00000000004c318d in do_for_each_entry_in_dir (dir=0x1fe1728, 
>     offset=<optimized out>, fn=0x4c1bc0 <do_one_ref>, cb_data=0x7fffc0b4d7c0)
>     at refs.c:672
> #5  0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf4d8, dir2=0x1fd6318, 
>     cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 <do_one_ref>) at refs.c:716
> #6  0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf1f8, dir2=0x1fd62d8, 
>     cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 <do_one_ref>) at refs.c:716
> #7  0x00000000004c3540 in do_for_each_entry (refs=refs@entry=0x7a2800 <ref_cache>, 
>     base=base@entry=0x509cc6 "", cb_data=cb_data@entry=0x7fffc0b4d7c0, 
>     fn=0x4c1bc0 <do_one_ref>) at refs.c:1689
> #8  0x00000000004c3ff8 in do_for_each_ref (cb_data=cb_data@entry=0x0, flags=1, trim=0, 
>     fn=fn@entry=0x424120 <get_name>, base=0x509cc6 "", refs=0x7a2800 <ref_cache>)
>     at refs.c:1724
> #9  for_each_rawref (fn=fn@entry=0x424120 <get_name>, cb_data=cb_data@entry=0x0)
>     at refs.c:1873
> #10 0x0000000000424f5b in cmd_describe (argc=0, argv=0x7fffc0b4ddc0, prefix=0x0)
>     at builtin/describe.c:466
> #11 0x000000000040596d in run_builtin (argv=0x7fffc0b4ddc0, argc=1, 
>     p=0x760b40 <commands.21352+576>) at git.c:291
> #12 handle_internal_command (argc=1, argv=0x7fffc0b4ddc0) at git.c:453
> #13 0x0000000000404d6e in run_argv (argv=0x7fffc0b4dc78, argcp=0x7fffc0b4dc5c)
>     at git.c:499
> #14 main (argc=1, av=<optimized out>) at git.c:575
> (gdb) 

According to `git bisect`, the first bad commit is:

commit 9a489f3c17d6c974b18c47cf406404ca2a721c87
Author: Michael Haggerty <mhagger@alum.mit.edu>
Date:   Mon Apr 22 21:52:22 2013 +0200

    refs: extract a function peel_entry()

The crash happens only in repositories that have at least one replaced
object in the branch's history. Running `git --no-replace-objects
describe` avoids the crash.

The crash happens only if there are any tags under .git/refs/tags/ that
do not exist in .git/packed-refs, or if I remove all "peeled" lines from
.git/packed-refs (including the '#' line; /^[#^]/d).

A quick way to reproduce this with git.git master is:

git tag -f test-tag HEAD~10
git replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \
  | sed 's/@/@test/' | git hash-object --stdin -t commit -w)
./git describe

-- 
Mantas Mikulėnas <grawity@gmail.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Segfault in `git describe`
  2013-07-13 13:27 Segfault in `git describe` Mantas Mikulėnas
@ 2013-07-15 13:03 ` Michael Haggerty
  2013-07-15 13:31   ` Mantas Mikulėnas
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Haggerty @ 2013-07-15 13:03 UTC (permalink / raw)
  To: Mantas Mikulėnas; +Cc: git

On 07/13/2013 03:27 PM, Mantas Mikulėnas wrote:
> I have a clone of linux.git with various stuff added to it (remotes for
> 'stable' and 'next', a bunch of local tags, and historical repositories
> imported using `git replace`).
> 
> Yesterday, I noticed that `git describe`, built from git.git master
> (v1.8.3.2-804-g0da7a53, gcc 4.8) would simply crash when run in that
> repository, with the following backtrace:
> 
>> Program terminated with signal 11, Segmentation fault.
>> #0  0x00000000004c39dc in hashcpy (sha_src=0x1c <Address 0x1c out of bounds>, 
>>     sha_dst=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022")
>>     at cache.h:694
>> 694		memcpy(sha_dst, sha_src, 20);
>> (gdb) bt
>> #0  0x00000000004c39dc in hashcpy (sha_src=0x1c <Address 0x1c out of bounds>, 
>>     sha_dst=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022")
>>     at cache.h:694
>> #1  peel_ref (refname=refname@entry=0x1fe2d10 "refs/tags/next-20130607", 
>>     sha1=sha1@entry=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022") at refs.c:1586
>> #2  0x0000000000424194 in get_name (path=0x1fe2d10 "refs/tags/next-20130607", 
>>     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=<optimized out>, 
>>     cb_data=<optimized out>) at builtin/describe.c:156
>> #3  0x00000000004c1c21 in do_one_ref (entry=0x1fe2ce0, cb_data=0x7fffc0b4d7c0)
>>     at refs.c:646
>> #4  0x00000000004c318d in do_for_each_entry_in_dir (dir=0x1fe1728, 
>>     offset=<optimized out>, fn=0x4c1bc0 <do_one_ref>, cb_data=0x7fffc0b4d7c0)
>>     at refs.c:672
>> #5  0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf4d8, dir2=0x1fd6318, 
>>     cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 <do_one_ref>) at refs.c:716
>> #6  0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf1f8, dir2=0x1fd62d8, 
>>     cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 <do_one_ref>) at refs.c:716
>> #7  0x00000000004c3540 in do_for_each_entry (refs=refs@entry=0x7a2800 <ref_cache>, 
>>     base=base@entry=0x509cc6 "", cb_data=cb_data@entry=0x7fffc0b4d7c0, 
>>     fn=0x4c1bc0 <do_one_ref>) at refs.c:1689
>> #8  0x00000000004c3ff8 in do_for_each_ref (cb_data=cb_data@entry=0x0, flags=1, trim=0, 
>>     fn=fn@entry=0x424120 <get_name>, base=0x509cc6 "", refs=0x7a2800 <ref_cache>)
>>     at refs.c:1724
>> #9  for_each_rawref (fn=fn@entry=0x424120 <get_name>, cb_data=cb_data@entry=0x0)
>>     at refs.c:1873
>> #10 0x0000000000424f5b in cmd_describe (argc=0, argv=0x7fffc0b4ddc0, prefix=0x0)
>>     at builtin/describe.c:466
>> #11 0x000000000040596d in run_builtin (argv=0x7fffc0b4ddc0, argc=1, 
>>     p=0x760b40 <commands.21352+576>) at git.c:291
>> #12 handle_internal_command (argc=1, argv=0x7fffc0b4ddc0) at git.c:453
>> #13 0x0000000000404d6e in run_argv (argv=0x7fffc0b4dc78, argcp=0x7fffc0b4dc5c)
>>     at git.c:499
>> #14 main (argc=1, av=<optimized out>) at git.c:575
>> (gdb) 
> 
> According to `git bisect`, the first bad commit is:
> 
> commit 9a489f3c17d6c974b18c47cf406404ca2a721c87
> Author: Michael Haggerty <mhagger@alum.mit.edu>
> Date:   Mon Apr 22 21:52:22 2013 +0200
> 
>     refs: extract a function peel_entry()
> 
> The crash happens only in repositories that have at least one replaced
> object in the branch's history. Running `git --no-replace-objects
> describe` avoids the crash.
> 
> The crash happens only if there are any tags under .git/refs/tags/ that
> do not exist in .git/packed-refs, or if I remove all "peeled" lines from
> .git/packed-refs (including the '#' line; /^[#^]/d).
> 
> A quick way to reproduce this with git.git master is:
> 
> git tag -f test-tag HEAD~10
> git replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \
>   | sed 's/@/@test/' | git hash-object --stdin -t commit -w)
> ./git describe

Thanks for the bug report.

I think the cause of this bug is that peel_entry() is causing a nested
call to do_for_each_entry() to look up the replace reference, which
resets current_ref to NULL between the test and the dereference of
current_ref in peel_ref().

Unfortunately, I cannot reproduce the failure by following your recipe
(though I didn't have a lot of time yet for this).  I suppose that my
repo starts out in a slightly different state than yours and therefore I
don't get the same results.  If you could find a recipe to reproduce the
problem, starting either with an empty repo, or perhaps a fresh clone of
git.git, and double-check that you don't have any unusual config options
that might be affecting things, that would be very helpful.

I might have more time to look at this tonight.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Segfault in `git describe`
  2013-07-15 13:03 ` Michael Haggerty
@ 2013-07-15 13:31   ` Mantas Mikulėnas
  2013-07-15 15:24     ` [PATCH] do_one_ref(): save and restore value of current_ref Michael Haggerty
  0 siblings, 1 reply; 8+ messages in thread
From: Mantas Mikulėnas @ 2013-07-15 13:31 UTC (permalink / raw)
  To: Michael Haggerty; +Cc: git

On Mon, Jul 15, 2013 at 4:03 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 07/13/2013 03:27 PM, Mantas Mikulėnas wrote:
>> I have a clone of linux.git with various stuff added to it (remotes for
>> 'stable' and 'next', a bunch of local tags, and historical repositories
>> imported using `git replace`).
>>
>> Yesterday, I noticed that `git describe`, built from git.git master
>> (v1.8.3.2-804-g0da7a53, gcc 4.8) would simply crash when run in that
>> repository, with the following backtrace:
>>
>>> Program terminated with signal 11, Segmentation fault.
>>> #0  0x00000000004c39dc in hashcpy (sha_src=0x1c <Address 0x1c out of bounds>,
>>>     sha_dst=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022")
>>>     at cache.h:694
>>> 694          memcpy(sha_dst, sha_src, 20);
>>> (gdb) bt
>>> #0  0x00000000004c39dc in hashcpy (sha_src=0x1c <Address 0x1c out of bounds>,
>>>     sha_dst=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022")
>>>     at cache.h:694
>>> #1  peel_ref (refname=refname@entry=0x1fe2d10 "refs/tags/next-20130607",
>>>     sha1=sha1@entry=0x7fffc0b4d610 "\242\271\301\366 \201&\346\337l\002B\214P\037\210ShX\022") at refs.c:1586
>>> #2  0x0000000000424194 in get_name (path=0x1fe2d10 "refs/tags/next-20130607",
>>>     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=<optimized out>,
>>>     cb_data=<optimized out>) at builtin/describe.c:156
>>> #3  0x00000000004c1c21 in do_one_ref (entry=0x1fe2ce0, cb_data=0x7fffc0b4d7c0)
>>>     at refs.c:646
>>> #4  0x00000000004c318d in do_for_each_entry_in_dir (dir=0x1fe1728,
>>>     offset=<optimized out>, fn=0x4c1bc0 <do_one_ref>, cb_data=0x7fffc0b4d7c0)
>>>     at refs.c:672
>>> #5  0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf4d8, dir2=0x1fd6318,
>>>     cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 <do_one_ref>) at refs.c:716
>>> #6  0x00000000004c33d1 in do_for_each_entry_in_dirs (dir1=0x1fdf1f8, dir2=0x1fd62d8,
>>>     cb_data=0x7fffc0b4d7c0, fn=0x4c1bc0 <do_one_ref>) at refs.c:716
>>> #7  0x00000000004c3540 in do_for_each_entry (refs=refs@entry=0x7a2800 <ref_cache>,
>>>     base=base@entry=0x509cc6 "", cb_data=cb_data@entry=0x7fffc0b4d7c0,
>>>     fn=0x4c1bc0 <do_one_ref>) at refs.c:1689
>>> #8  0x00000000004c3ff8 in do_for_each_ref (cb_data=cb_data@entry=0x0, flags=1, trim=0,
>>>     fn=fn@entry=0x424120 <get_name>, base=0x509cc6 "", refs=0x7a2800 <ref_cache>)
>>>     at refs.c:1724
>>> #9  for_each_rawref (fn=fn@entry=0x424120 <get_name>, cb_data=cb_data@entry=0x0)
>>>     at refs.c:1873
>>> #10 0x0000000000424f5b in cmd_describe (argc=0, argv=0x7fffc0b4ddc0, prefix=0x0)
>>>     at builtin/describe.c:466
>>> #11 0x000000000040596d in run_builtin (argv=0x7fffc0b4ddc0, argc=1,
>>>     p=0x760b40 <commands.21352+576>) at git.c:291
>>> #12 handle_internal_command (argc=1, argv=0x7fffc0b4ddc0) at git.c:453
>>> #13 0x0000000000404d6e in run_argv (argv=0x7fffc0b4dc78, argcp=0x7fffc0b4dc5c)
>>>     at git.c:499
>>> #14 main (argc=1, av=<optimized out>) at git.c:575
>>> (gdb)
>>
>> According to `git bisect`, the first bad commit is:
>>
>> commit 9a489f3c17d6c974b18c47cf406404ca2a721c87
>> Author: Michael Haggerty <mhagger@alum.mit.edu>
>> Date:   Mon Apr 22 21:52:22 2013 +0200
>>
>>     refs: extract a function peel_entry()
>>
>> The crash happens only in repositories that have at least one replaced
>> object in the branch's history. Running `git --no-replace-objects
>> describe` avoids the crash.
>>
>> The crash happens only if there are any tags under .git/refs/tags/ that
>> do not exist in .git/packed-refs, or if I remove all "peeled" lines from
>> .git/packed-refs (including the '#' line; /^[#^]/d).
>>
>> A quick way to reproduce this with git.git master is:
>>
>> git tag -f test-tag HEAD~10
>> git replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \
>>   | sed 's/@/@test/' | git hash-object --stdin -t commit -w)
>> ./git describe
>
> Thanks for the bug report.
>
> I think the cause of this bug is that peel_entry() is causing a nested
> call to do_for_each_entry() to look up the replace reference, which
> resets current_ref to NULL between the test and the dereference of
> current_ref in peel_ref().
>
> Unfortunately, I cannot reproduce the failure by following your recipe
> (though I didn't have a lot of time yet for this).  I suppose that my
> repo starts out in a slightly different state than yours and therefore I
> don't get the same results.  If you could find a recipe to reproduce the
> problem, starting either with an empty repo, or perhaps a fresh clone of
> git.git, and double-check that you don't have any unusual config options
> that might be affecting things, that would be very helpful.

Hmm, yes, just creating a new tag doesn't break in another
freshly-cloned repo, either.

However,

> …or if I remove all "peeled" lines from .git/packed-refs (including the '#' line; /^[#^]/d).

still works for reproducing the crash. When packed-refs does not have
any peeled refs, older git versions do it manually (I assume for
compatibility with even older git versions), while the latest one
crashes. This recipe should work:

git pack-refs --all --prune
sed -i '/^[#^]/d' .git/packed-refs
git replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \
    | sed 's/@/@test/' | git hash-object --stdin -t commit -w)
~/src/git/git describe

--
Mantas Mikulėnas <grawity@gmail.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] do_one_ref(): save and restore value of current_ref
  2013-07-15 13:31   ` Mantas Mikulėnas
@ 2013-07-15 15:24     ` Michael Haggerty
  2013-07-18  4:03       ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Haggerty @ 2013-07-15 15:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Mantas Mikulėnas, Michael Haggerty

If do_one_ref() is called recursively, then the inner call should not
permanently overwrite the value stored in current_ref by the outer
call.  Aside from the tiny optimization loss, peel_ref() expects the
value of current_ref not to change across a call to peel_entry().  But
in the presence of replace references that assumption could be
violated by a recursive call to do_one_ref:

do_for_each_entry()
  do_one_ref()
    builtin/describe.c:get_name()
      peel_ref()
        peel_entry()
          peel_object ()
            deref_tag_noverify()
              parse_object()
                lookup_replace_object()
                  do_lookup_replace_object()
                    prepare_replace_object()
                      do_for_each_ref()
                        do_for_each_entry()
                          do_for_each_entry_in_dir()
                            do_one_ref()

The inner call to do_one_ref() was unconditionally setting current_ref
to NULL when it was done, causing peel_ref() to perform an invalid
memory access.

So change do_one_ref() to save the old value of current_ref before
overwriting it, and restore the old value afterward rather than
setting it to NULL.

Reported by: Mantas Mikulėnas <grawity@gmail.com>

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 4302206..222baf2 100644
--- a/refs.c
+++ b/refs.c
@@ -634,7 +634,9 @@ struct ref_entry_cb {
 static int do_one_ref(struct ref_entry *entry, void *cb_data)
 {
 	struct ref_entry_cb *data = cb_data;
+	struct ref_entry *old_current_ref;
 	int retval;
+
 	if (prefixcmp(entry->name, data->base))
 		return 0;
 
@@ -642,10 +644,12 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)
 	      !ref_resolves_to_object(entry))
 		return 0;
 
+	/* Store the old value, in case this is a recursive call: */
+	old_current_ref = current_ref;
 	current_ref = entry;
 	retval = data->fn(entry->name + data->trim, entry->u.value.sha1,
 			  entry->flag, data->cb_data);
-	current_ref = NULL;
+	current_ref = old_current_ref;
 	return retval;
 }
 
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] do_one_ref(): save and restore value of current_ref
  2013-07-15 15:24     ` [PATCH] do_one_ref(): save and restore value of current_ref Michael Haggerty
@ 2013-07-18  4:03       ` Junio C Hamano
  2013-07-19 17:43         ` Michael Haggerty
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2013-07-18  4:03 UTC (permalink / raw)
  To: Michael Haggerty; +Cc: git, Mantas Mikulėnas

Michael Haggerty <mhagger@alum.mit.edu> writes:

> If do_one_ref() is called recursively, then the inner call should not
> permanently overwrite the value stored in current_ref by the outer
> call.  Aside from the tiny optimization loss, peel_ref() expects the
> value of current_ref not to change across a call to peel_entry().  But
> in the presence of replace references that assumption could be
> violated by a recursive call to do_one_ref:
>
> do_for_each_entry()
>   do_one_ref()
>     builtin/describe.c:get_name()
>       peel_ref()
>         peel_entry()
>           peel_object ()
>             deref_tag_noverify()
>               parse_object()
>                 lookup_replace_object()
>                   do_lookup_replace_object()
>                     prepare_replace_object()
>                       do_for_each_ref()
>                         do_for_each_entry()
>                           do_for_each_entry_in_dir()
>                             do_one_ref()
>
> The inner call to do_one_ref() was unconditionally setting current_ref
> to NULL when it was done, causing peel_ref() to perform an invalid
> memory access.
>
> So change do_one_ref() to save the old value of current_ref before
> overwriting it, and restore the old value afterward rather than
> setting it to NULL.
>
> Reported by: Mantas Mikulėnas <grawity@gmail.com>
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

Thanks.

s/Reported by:/Reported-by:/ and lose the extra blank line after it?

I wonder if we can have an easy reproduction recipe in our tests.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] do_one_ref(): save and restore value of current_ref
  2013-07-18  4:03       ` Junio C Hamano
@ 2013-07-19 17:43         ` Michael Haggerty
  2013-07-19 19:34           ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Haggerty @ 2013-07-19 17:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Mantas Mikulėnas

On 07/17/2013 09:03 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> If do_one_ref() is called recursively, then the inner call should not
>> permanently overwrite the value stored in current_ref by the outer
>> call.  Aside from the tiny optimization loss, peel_ref() expects the
>> value of current_ref not to change across a call to peel_entry().  But
>> in the presence of replace references that assumption could be
>> violated by a recursive call to do_one_ref:
>>
>> do_for_each_entry()
>>   do_one_ref()
>>     builtin/describe.c:get_name()
>>       peel_ref()
>>         peel_entry()
>>           peel_object ()
>>             deref_tag_noverify()
>>               parse_object()
>>                 lookup_replace_object()
>>                   do_lookup_replace_object()
>>                     prepare_replace_object()
>>                       do_for_each_ref()
>>                         do_for_each_entry()
>>                           do_for_each_entry_in_dir()
>>                             do_one_ref()
>>
>> The inner call to do_one_ref() was unconditionally setting current_ref
>> to NULL when it was done, causing peel_ref() to perform an invalid
>> memory access.
>>
>> So change do_one_ref() to save the old value of current_ref before
>> overwriting it, and restore the old value afterward rather than
>> setting it to NULL.
>>
>> Reported by: Mantas Mikulėnas <grawity@gmail.com>
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
> Thanks.
> 
> s/Reported by:/Reported-by:/ and lose the extra blank line after it?

ACK, sorry I got the notation wrong.

> I wonder if we can have an easy reproduction recipe in our tests.

I could reproduce the problem by following the recipe provided by Mantas
upthread (or at least something very close to it; I can't find the
script that I was using):

> git pack-refs --all --prune
> sed -i '/^[#^]/d' .git/packed-refs
> git replace -f HEAD $(git --no-replace-objects cat-file commit HEAD \
>     | sed 's/@/@test/' | git hash-object --stdin -t commit -w)
> ~/src/git/git describe

It would be good to document this in the commit message, but I don't
think it is necessary to have a test for it in the test suite because I
don't think it is the kind of bug that will reappear.

I sent the patch shortly before leaving for a trip so I didn't have time
to make it as complete as I would have liked.  But given that the
problem was already in master, and the fix is pretty simple, I wanted to
send the fix right away.  When I have some time I can fix it up better,
or feel free to manhandle the patch and/or commit message yourself if
you prefer.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] do_one_ref(): save and restore value of current_ref
  2013-07-19 17:43         ` Michael Haggerty
@ 2013-07-19 19:34           ` Junio C Hamano
  2013-07-24 14:35             ` Michael Haggerty
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2013-07-19 19:34 UTC (permalink / raw)
  To: Michael Haggerty; +Cc: git, Mantas Mikulėnas

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I sent the patch shortly before leaving for a trip so I didn't have time
> to make it as complete as I would have liked.  But given that the
> problem was already in master, and the fix is pretty simple, I wanted to
> send the fix right away.  When I have some time I can fix it up better,

That is very much appreciated.  How would you describe this fix in a
two-to-three line paragraph in Release Notes?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] do_one_ref(): save and restore value of current_ref
  2013-07-19 19:34           ` Junio C Hamano
@ 2013-07-24 14:35             ` Michael Haggerty
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Haggerty @ 2013-07-24 14:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Mantas Mikulėnas

On 07/19/2013 12:34 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I sent the patch shortly before leaving for a trip so I didn't have time
>> to make it as complete as I would have liked.  But given that the
>> problem was already in master, and the fix is pretty simple, I wanted to
>> send the fix right away.  When I have some time I can fix it up better,
> 
> That is very much appreciated.  How would you describe this fix in a
> two-to-three line paragraph in Release Notes?

How about:

    Fix a NULL-pointer dereference during nested iterations over
    references (for example, when replace references are being used).

Unfortunately I don't have time now to audit the code more carefully to
figure out what other circumstances might have triggered the bug.

Hope that helps,
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-07-24 14:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-13 13:27 Segfault in `git describe` Mantas Mikulėnas
2013-07-15 13:03 ` Michael Haggerty
2013-07-15 13:31   ` Mantas Mikulėnas
2013-07-15 15:24     ` [PATCH] do_one_ref(): save and restore value of current_ref Michael Haggerty
2013-07-18  4:03       ` Junio C Hamano
2013-07-19 17:43         ` Michael Haggerty
2013-07-19 19:34           ` Junio C Hamano
2013-07-24 14:35             ` Michael Haggerty

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.