Re: [PATCH] fsck: check results of lookup_blob() and lookup_tree() for NULL
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 4, 2017, 04:00 UTC
- Message-ID
- <xmqqh8vfftp7.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <54f5877b-a143-11c2-d8f6-ff28ed9e7e38@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 17 quoted lines
> 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.
Show 33 quoted lines
>
> 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);