[PATCH] fix multiple issues in index-pack

[PATCH] fix multiple issues in index-pack

[Nicolas Pitre]

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.

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");

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

[Jeff King]

On Mon, Oct 20, 2008 at 04:46:19PM -0400, Nicolas Pitre wrote:

>  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

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

[Marco Roeland]

On Monday October 20th 2008 at 16:46 Nicolas Pitre wrote:

> 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

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

[Junio C Hamano]

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.