Re: [PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL

[Junio C Hamano <gitster@pobox.com>]

René Scharfe <l.s.r@web.de> writes:

> Am 03.10.2017 um 14:51 schrieb René Scharfe:
>> Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:
>>> Furthermore, fsck.c:fsck_walk_tree() does the same "immediately
>>> reference the object member in lookup_blob()'s and lookup_tree()'s
>>> return value" thing.  I think those should receive the same treatment
>>> as well.
>> 
>> Hmm, are put_object_name() and all the walk() implementations ready for
>> a NULL object handed to them?  Or would we rather need to error out
>> right there?
> How about this?
>
> -- >8 --
> lookup_blob() and lookup_tree() can return NULL if they find an object
> of an unexpected type.  Error out of fsck_walk_tree() in that case, like
> we do when encountering a bad file mode.  An error message is already
> shown by object_as_type(), which gets called by the lookup functions.

The result from options->walk() is checked, and among the callbacks
that are assigned to the .walk field:

 - mark_object() in builtin/fsck.c gives its own error message to diagnose broken link
   and returns 1;

 - mark_used() in builtin/fsck.c silently returns 1;

 - mark_link() in builtin/index-pack.c does the same; and

 - check_object() in builtin/unpack-objects.c does the same,

when they see a NULL object.

This patch may avoid the "unexpected behaviour" coming from
expecting that &((struct tree *)NULL)->object == NULL the current
code does, but it also changes the behaviour.  The loop used to
diagnose a fishy entry in the tree we are walking, and kept checking
the remaining entries in the tree.  You now immediately return, not
seeing if the later entries in the tree are good and losing objects
that are referenced by these entries as dangling.

I am not sure if this is a good change.  I suspect that the "bad
mode" handling should be made less severe instead.

>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  fsck.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 2ad00fc4d0..561a13ac27 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -358,14 +358,20 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
>  			continue;
>  
>  		if (S_ISDIR(entry.mode)) {
> -			obj = &lookup_tree(entry.oid)->object;
> +			struct tree *tree = lookup_tree(entry.oid);
> +			if (!tree)
> +				return -1;
> +			obj = &tree->object;
>  			if (name)
>  				put_object_name(options, obj, "%s%s/", name,
>  					entry.path);
>  			result = options->walk(obj, OBJ_TREE, data, options);
>  		}
>  		else if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {
> -			obj = &lookup_blob(entry.oid)->object;
> +			struct blob *blob = lookup_blob(entry.oid);
> +			if (!blob)
> +				return -1;
> +			obj = &blob->object;
>  			if (name)
>  				put_object_name(options, obj, "%s%s", name,
>  					entry.path);