Re: [PATCH 2/3] fsck: actually detect bad file modes in trees

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2022, 21:35 UTC
Message-ID: <xmqq4jyjkcij.fsf@gitster.g>
In-Reply-To: <YvQc9egGzwFomQbw@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> We use the normal tree_desc code to iterate over trees in fsck, meaning
> we only see the canonicalized modes it returns. And hence we'd never see
> anything unexpected, since it will coerce literally any garbage into one
> of our normal and accepted modes.

Wow. I did know canon_mode() deliberately discarding the extra permission bits on trees and blobs, but it was that bad to mark whatever it does not understand as a gitlink. That is simply horrible.

Show 5 quoted lines

> -	if (init_tree_desc_gently(&desc, buffer, size, 0)) {
> +	if (init_tree_desc_gently(&desc, buffer, size, TREE_DESC_RAW_MODES)) {
>  		retval += report(options, tree_oid, OBJ_TREE,
>  				 FSCK_MSG_BAD_TREE,
>  				 "cannot be parsed as a tree");

OK, so we'll let desc.entry.mode carry whatever bogus bit pattern we got out of buffer and the downstream code already knows what to do with them. That's a clean and minimum way to do this.

Thanks.

Show 25 quoted lines

> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index ab7f31f1dc..53c2aa10b7 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -364,6 +364,20 @@ test_expect_success 'tree entry with type mismatch' '
>  	test_i18ngrep ! "dangling blob" out
>  '
>  
> +test_expect_success 'tree entry with bogus mode' '
> +	test_when_finished "remove_object \$blob" &&
> +	test_when_finished "remove_object \$tree" &&
> +	blob=$(echo blob | git hash-object -w --stdin) &&
> +	blob_oct=$(echo $blob | hex2oct) &&
> +	tree=$(printf "100000 foo\0${blob_oct}" |
> +	       git hash-object -t tree --stdin -w --literally) &&
> +	git fsck 2>err &&
> +	cat >expect <<-EOF &&
> +	warning in tree $tree: badFilemode: contains bad file modes
> +	EOF
> +	test_cmp expect err
> +'
> +
>  test_expect_success 'tag pointing to nonexistent' '
>  	badoid=$(test_oid deadbeef) &&
>  	cat >invalid-tag <<-EOF &&