Re: [RFC Patch] Preventing corrupt objects from entering the repository

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2008, 00:00 UTC
Message-ID: <7vmyq8cqfn.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080210175812.GB12162@auto.tuwien.ac.at>

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:

> So I tried to fix the problem in another way: Instead of coping with 
> corrupt objects, stop them from entering the repository.

Good intention.  Very nice.

Show 6 quoted lines

> * add --strict option to unpack-objects (patch 1,8,9)
> * add --strict option to index-pack (patch 8,10)
>
>   Same as for unpack-objects, but without writting objects.
>
> * add config option for receive pack to enable checking (patch 11)

If this patch is any good, I strongly suspect it should not be just the default but should always be on. IOW no config is necessary. That would make the series a bit simpler, I guess.

Show 10 quoted lines

> From 76e86fe55345e633c910d6b8fe166e27c23c5aaf Mon Sep 17 00:00:00 2001
> From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> Date: Fri, 8 Feb 2008 08:51:38 +0100
> Subject: [PATCH 01/12] unpack-object: cache for non written objects
>
> Preventing objects with broken links entering the repository
> means, that write of some objects must be delayed.
>
> This patch adds a cache to keep the object data in memory. The delta
> resolving code must also search in the cache.

I have to wonder what the memory pressure in real-life usage will be like.

When an object is proven to be good, we should be able to free its buffer after writing it out, but would that be a good enough optimization we can make later on this code to keep its memory consumption manageable?

Show 22 quoted lines

> diff --git a/fsck.c b/fsck.c
> new file mode 100644
> index 0000000..089f775
> --- /dev/null
> +++ b/fsck.c
> @@ -0,0 +1,84 @@
> +#include "cache.h"
> +#include "object.h"
> +#include "blob.h"
> +#include "tree.h"
> +#include "tree-walk.h"
> +#include "commit.h"
> +#include "tag.h"
> +#include "fsck.h"
> +
> +static int fsck_walk_tree(struct tree* tree, fsck_walk_func walk, void* data)
> +{
> +	struct tree_desc desc;
> +	struct name_entry entry;
> +
> +	if(parse_tree(tree))
> +		return -1;

I noticed many coding style issues but I won't be mentioning them in this response.

It's a bit hard to see how these new set of functions relate to the original code in this patch series, because you add the new things that are initially not used anywhere independently, start referring to them in a separate patch and then remove the old related functions that are now unused. This style makes reviewing easier and harder at the same time...

Show 21 quoted lines

> From 80b22c3f2c3e13c207790a49646020c55b34bba7 Mon Sep 17 00:00:00 2001
> From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> Date: Fri, 8 Feb 2008 09:01:50 +0100
> Subject: [PATCH 03/12] fsck: move mark-reachable to fsck_walk
>
> Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> ---
>  builtin-fsck.c |   34 ++++++++++++++++++++++++----------
>  1 files changed, 24 insertions(+), 10 deletions(-)
>  ...
> +static int mark_object(struct object* obj, int type, void* data)
> +{
> +	if (!obj)
> +		return 0;
> +	if (obj->flags & REACHABLE)
> +		return 0;
> +	obj->flags |= REACHABLE;
> +	if (!obj->parsed)
> +		return 0;
> +	return fsck_walk(obj, mark_object, data);
> +}

Hmm. The return value 0 means Ok and negative is error? The reason we can say success if obj is NULL or it is not parsed yet is because...?

Show 6 quoted lines

> @@ -326,8 +344,6 @@ static int fsck_tree(struct tree *item)
>  		o_name = name;
>  		o_sha1 = sha1;
>  	}
> -	free(item->buffer);
> -	item->buffer = NULL;

Hmm. The reason you still need the buffer after you checked the contents of the tree in the loop is because you haven't actually checked the referents are Ok. But I do not see a corresponding free that releases this memory after you are actually done with the verification with fsck_walk() yet, so we leak this in the meantime?

Show 9 quoted lines

> @@ -375,8 +391,6 @@ static int fsck_commit(struct commit *commit)
>  	}
>  	if (memcmp(buffer, "author ", 7))
>  		return objerror(&commit->object, "invalid format - expected 'author' line");
> -	free(commit->buffer);
> -	commit->buffer = NULL;
>  	if (!commit->tree)
>  		return objerror(&commit->object, "could not load commit's tree %s", tree_sha1);
>  	if (!commit->parents && show_root)

Likewise.

Show 28 quoted lines

> From ce43251ef71962ff64fe138f1295c405ef6aaf65 Mon Sep 17 00:00:00 2001
> From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> Date: Fri, 8 Feb 2008 09:04:08 +0100
> Subject: [PATCH 04/12] fsck: move reachable object check to fsck_walk
>
> It handles NULL pointers in object references without crashing.
>
> Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> ---
>  builtin-fsck.c |   49 +++++++++++++++++++++++++++++--------------------
>  1 files changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/builtin-fsck.c b/builtin-fsck.c
> index 49e96ff..2c1e10f 100644
> --- a/builtin-fsck.c
> +++ b/builtin-fsck.c
> @@ -81,13 +81,39 @@ static int objwarning(struct object *obj, const char *err, ...)
>  	return -1;
>  }
>  
> +static int check_reachable_object_childs(struct object *obj, int type, void *data)
> +{
> +	struct object *parent = data;
> +	if (!obj) {
> +		printf("broken link from %7s %s\n",
> +			   typename(parent->type), sha1_to_hex(parent->sha1));
> +		printf("broken link from %7s %s\n",
> +			   (type==OBJ_ANY?"unknown":typename(type)), "unknown");

Hmm?  I am not sure what this part is reporting...

Show 6 quoted lines

> From ee11f771be1ef1c29725cb56ab3eb8dfe61ca25a Mon Sep 17 00:00:00 2001
> From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> Date: Fri, 8 Feb 2008 09:07:33 +0100
> Subject: [PATCH 06/12] create a common object checker code out of fsck
>
> The function provides a callback for reporting errors.

The same "add unused new stuff independently, later use it and then finally remove now unused old stuff" pattern is here. I am neutral to that patch style but it is a bit harder to see what is going on.

Most of the changes seem to be straight and sane copy-and-paste though.

Show 13 quoted lines

> From a8db4e754e717bac0b2462333d4145eac3452099 Mon Sep 17 00:00:00 2001
> From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> Date: Fri, 8 Feb 2008 09:14:14 +0100
> Subject: [PATCH 09/12] unpack-objects: prevent writing of inconsistent objects
>
> This patch introduces a strict mode, which ensures that:
> - no malformed object will be written
> - no object with broken links will be written
>
> The patch ensures this by delaying the write of all non blob object.
> These object are written, after all objects they link to are written.
>
> An error can only result in unreferenced objects.

Show 10 quoted lines

> diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
> index f18c7e8..3e906e4 100644
> --- a/builtin-unpack-objects.c
> +++ b/builtin-unpack-objects.c
> @@ -173,7 +250,6 @@ static void resolve_delta(unsigned nr, enum object_type type,
>  		die("failed to apply delta");
>  	free(delta);
>  	write_object(nr, type, result, result_size);
> -	free(result);
>  }

And this is freed later elsewhere?

Show 8 quoted lines

> @@ -203,7 +279,8 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,
>  
>  	if (!dry_run && buf)
>  		write_object(nr, type, buf, size);
> -	free(buf);
> +	else if (buf)
> +		free(buf);
>  }

You can always free NULL without checking.

Show 5 quoted lines

> @@ -356,6 +434,7 @@ static void unpack_all(void)
>  	if (!quiet)
>  		progress = start_progress("Unpacking objects", nr_objects);
>  	obj_list = xmalloc(nr_objects * sizeof(*obj_list));
> +	memset(obj_list, 0, nr_objects * sizeof(*obj_list));

Hmm, is this a fix to the 'master' independent from all the rest of your patches, or a new requirement?