threads / patch / 15986

patchRe: [PATCH] fix multiple issues in index-pack

Subject: Re: [PATCH] fix multiple issues in index-pack

## tl;dr

4 messages between Oct 20, 2008 and Oct 20, 2008. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Nicolas Pitre· Oct 20, 2008, 20:46 UTC · lore

[PATCH] fix multiple issues in index-pack

Since commit 9441b61dc5, two issues affected correct behavior of index-pack:

 1) The real_type of a delta object is the 'real_type' of its base, not
    the 'type' which can be a "delta type".  Consequence of this is a
    corrupted pack index file which only needs to be recreated with a 
    good index-pack command ('git verify-pack' will flag those).
 2( The code sequence:
        result->data = patch_delta(get_base_data(base), base->obj->size,
                                   delta_data, delta_size, &result->size);
    has two issues of its own since base->obj->size should instead be
    base->size as we want the size of the actual object data and not
    the size of the delta object it is represented by.  Except that 
    simply replacing base->obj->size with base->size won't make the
    code more correct as the C language doesn't enforce a particular 
    ordering for the evaluation of needed arguments for a function call,
    hence base->size could be pushed on the stack before get_base_data()
    which initializes base->size is called.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---

Damn... this one was subtle. And I'm still wondering how the hell the test suite is able to pass with this. I'll try to figure out why and come up with better tests.

Show changes to index-pack.c +5 −6
diff --git a/index-pack.c b/index-pack.c
index 0a917d7..e179bd9 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -514,15 +514,14 @@ static void *get_base_data(struct base_data *c)
 static void resolve_delta(struct object_entry *delta_obj,
 			  struct base_data *base, struct base_data *result)
 {
-	void *delta_data;
-	unsigned long delta_size;
+	void *base_data, *delta_data;
 
-	delta_obj->real_type = base->obj->type;
+	delta_obj->real_type = base->obj->real_type;
 	delta_data = get_data_from_pack(delta_obj);
-	delta_size = delta_obj->size;
+	base_data = get_base_data(base);
 	result->obj = delta_obj;
-	result->data = patch_delta(get_base_data(base), base->obj->size,
-				   delta_data, delta_size, &result->size);
+	result->data = patch_delta(base_data, base->size,
+				   delta_data, delta_obj->size, &result->size);
 	free(delta_data);
 	if (!result->data)
 		bad_object(delta_obj->idx.offset, "failed to apply delta");
Jeff King· Oct 20, 2008, 20:56 UTC · re: Nicolas Pitre · lore
On Mon, Oct 20, 2008 at 04:46:19PM -0400, Nicolas Pitre wrote:
Show 8 quoted lines
>  2( The code sequence:
> 
>         result->data = patch_delta(get_base_data(base), base->obj->size,
>                                    delta_data, delta_size, &result->size);
> 
>     has two issues of its own since base->obj->size should instead be
>     base->size as we want the size of the actual object data and not
>     the size of the delta object it is represented by.  Except that 
This one fixes my problem.
Tested-by: Jeff King <peff@peff.net>
-Peff
Marco Roeland· Oct 20, 2008, 20:59 UTC · re: Nicolas Pitre · lore
On Monday October 20th 2008 at 16:46 Nicolas Pitre wrote:
Show 23 quoted lines
> Since commit 9441b61dc5, two issues affected correct behavior of 
> index-pack:
> 
>  1) The real_type of a delta object is the 'real_type' of its base, not
>     the 'type' which can be a "delta type".  Consequence of this is a
>     corrupted pack index file which only needs to be recreated with a 
>     good index-pack command ('git verify-pack' will flag those).
> 
>  2( The code sequence:
> 
>         result->data = patch_delta(get_base_data(base), base->obj->size,
>                                    delta_data, delta_size, &result->size);
> 
>     has two issues of its own since base->obj->size should instead be
>     base->size as we want the size of the actual object data and not
>     the size of the delta object it is represented by.  Except that 
>     simply replacing base->obj->size with base->size won't make the
>     code more correct as the C language doesn't enforce a particular 
>     ordering for the evaluation of needed arguments for a function call,
>     hence base->size could be pushed on the stack before get_base_data()
>     which initializes base->size is called.
> 
> Signed-off-by: Nicolas Pitre <nico@cam.org>
This patch works for me. Thanks and good detective work.
-- 
Marco Roeland
Junio C Hamano· Oct 20, 2008, 21:31 UTC · re: Nicolas Pitre · lore
Nicolas Pitre <nico@cam.org> writes:
> Damn... this one was subtle.  And I'm still wondering how the hell the 
> test suite is able to pass with this.  I'll try to figure out why and 
> come up with better tests.
Thanks; much appreciated.

← back to recent threads