Re: [PATCH] index-pack: always zero-initialize object_entry list

Jeff King [off-list ref] writes:

Commit 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03) added a "delta_depth" counter
to each "struct object_entry". Initially, all object entries
have their depth set to 0; in resolve_delta, we then set the
depth of each delta to "base + 1". Base entries never have
their depth touched, and remain at 0.

This patch causes index-pack to fail on the pack that triggered the
whole discussion.  More in a minute in another side thread, but
meanwhile: NAK until we understand what is really going on here.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Tue, Mar 19, 2013 at 04:33:42PM +0100, Thomas Rast wrote:

Jeff King [off-list ref] writes:

quoted

Commit 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03) added a "delta_depth" counter
to each "struct object_entry". Initially, all object entries
have their depth set to 0; in resolve_delta, we then set the
depth of each delta to "base + 1". Base entries never have
their depth touched, and remain at 0.

This patch causes index-pack to fail on the pack that triggered the
whole discussion.  More in a minute in another side thread, but
meanwhile: NAK until we understand what is really going on here.

Odd; that's what I was testing with, and it worked fine.

Let me double-check that I didn't screw up my tests. I initially did
something more like:

  if (is_delta_type(base->obj->type))
          delta_obj->delta_depth = base->obj->delta_depth + 1;
  else
          delta_obj->delta_depth = 1;

and I'm wondering if I screwed up testing the revised version.

-Peff

On Tue, Mar 19, 2013 at 11:43:53AM -0400, Jeff King wrote:

On Tue, Mar 19, 2013 at 04:33:42PM +0100, Thomas Rast wrote:

quoted

Jeff King [off-list ref] writes:

quoted

Commit 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03) added a "delta_depth" counter
to each "struct object_entry". Initially, all object entries
have their depth set to 0; in resolve_delta, we then set the
depth of each delta to "base + 1". Base entries never have
their depth touched, and remain at 0.

This patch causes index-pack to fail on the pack that triggered the
whole discussion.  More in a minute in another side thread, but
meanwhile: NAK until we understand what is really going on here.

Odd; that's what I was testing with, and it worked fine.

Ah, interesting. I built the fix on top of d1a0ed1, the first commit
that shows the problem. And it works fine there. But when it is
forward-ported to the current master, it breaks as you saw.

More bisection fun.

-Peff

On Tue, Mar 19, 2013 at 11:52:44AM -0400, Jeff King wrote:

quoted

quoted

quoted

Commit 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03) added a "delta_depth" counter
to each "struct object_entry". Initially, all object entries
have their depth set to 0; in resolve_delta, we then set the
depth of each delta to "base + 1". Base entries never have
their depth touched, and remain at 0.

This patch causes index-pack to fail on the pack that triggered the
whole discussion.  More in a minute in another side thread, but
meanwhile: NAK until we understand what is really going on here.

Odd; that's what I was testing with, and it worked fine.

Ah, interesting. I built the fix on top of d1a0ed1, the first commit
that shows the problem. And it works fine there. But when it is
forward-ported to the current master, it breaks as you saw.

More bisection fun.

So after bisecting, I realize that it is indeed broken on top of
d1a0ed1. I have no idea why I didn't notice that before; I'm guessing it
was because I was running it under valgrind and paying attention only to
valgrind errors.

Anyway, the problem is simple and stupid. The original object array is
not nr_objects item long; it is (nr_objects + 1) long, though I'm not
clear why (1-indexing?). So my previous patch was zeroing the final
entry, which was supposed to contain actual data. Oops.

Here's the corrected patch.

-- >8 --
Subject: [PATCH] index-pack: always zero-initialize object_entry list

Commit 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03) added a "delta_depth" counter
to each "struct object_entry". Initially, all object entries
have their depth set to 0; in resolve_delta, we then set the
depth of each delta to "base + 1". Base entries never have
their depth touched, and remain at 0.

To ensure that all depths start at 0, that commit changed
calls to xmalloc the object_entry list into calls to
xcalloc.  However, it forgot that we grow the list with
xrealloc later. These extra entries are used when we add an
object from elsewhere pack to complete a thin pack. If we
add a non-delta object, its depth value will just be
uninitialized heap data.

This patch fixes it by zero-initializing entries we add to
the objects list via the xrealloc.

Signed-off-by: Jeff King <redacted>
---
 builtin/index-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 43d364b..5860085 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -1107,6 +1107,8 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
 		objects = xrealloc(objects,
 				   (nr_objects + nr_unresolved + 1)
 				   * sizeof(*objects));
+		memset(objects + nr_objects + 1, 0,
+		       nr_unresolved * sizeof(*objects));
 		f = sha1fd(output_fd, curr_pack);
 		fix_unresolved_deltas(f, nr_unresolved);
 		sprintf(msg, _("completed with %d local objects"),

-- 
1.8.2.22.g4863f63

On Tue, Mar 19, 2013 at 12:17 PM, Jeff King [off-list ref] wrote:

To ensure that all depths start at 0, that commit changed
calls to xmalloc the object_entry list into calls to
xcalloc.  However, it forgot that we grow the list with
xrealloc later. These extra entries are used when we add an
object from elsewhere pack to complete a thin pack. If we

s/elsewhere pack/pack/

add a non-delta object, its depth value will just be
uninitialized heap data.

On Wed, Mar 20, 2013 at 03:12:07PM -0400, Eric Sunshine wrote:

On Tue, Mar 19, 2013 at 12:17 PM, Jeff King [off-list ref] wrote:

quoted

To ensure that all depths start at 0, that commit changed
calls to xmalloc the object_entry list into calls to
xcalloc.  However, it forgot that we grow the list with
xrealloc later. These extra entries are used when we add an
object from elsewhere pack to complete a thin pack. If we

s/elsewhere pack/pack/

I think it is supposed to be s/elsewhere pack/elsewhere/.

Thanks for noticing.

-Peff

On Wed, Mar 20, 2013 at 3:13 PM, Jeff King [off-list ref] wrote:

On Wed, Mar 20, 2013 at 03:12:07PM -0400, Eric Sunshine wrote:

quoted

On Tue, Mar 19, 2013 at 12:17 PM, Jeff King [off-list ref] wrote:

quoted

To ensure that all depths start at 0, that commit changed
calls to xmalloc the object_entry list into calls to
xcalloc.  However, it forgot that we grow the list with
xrealloc later. These extra entries are used when we add an
object from elsewhere pack to complete a thin pack. If we

s/elsewhere pack/pack/

I think it is supposed to be s/elsewhere pack/elsewhere/.

Sorry, yes.

-- ES