Re: [PATCH] archive: Store checksum correctly

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2019, 16:49 UTC
Message-ID: <xmqqd0i0u253.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190723025736.23036-1-mattst88@gmail.com>

Matt Turner <mattst88@gmail.com> writes:

Show 31 quoted lines

> tar2sqfs (part of https://github.com/topics/tar2sqfs) rejects tarballs
> made with git archive with the message
>
>     invalid tar header checksum!
>
> tar2sqfs recomputes the tarball's checksum to verify it. Its checksum
> implementation agrees with GNU tar, which contains a comment that states
>
>     Fill in the checksum field.  It's formatted differently from the
>     other fields: it has [6] digits, a null, then a space ...
>
> Correcting this allows tar2sqfs to correctly process the tarballs made
> by git archive.
>
> Signed-off-by: Matt Turner <mattst88@gmail.com>
> ---
>  archive-tar.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index 3e53aac1e6..f9a157bfd1 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -215,7 +215,9 @@ static void prepare_header(struct archiver_args *args,
>  	memcpy(header->magic, "ustar", 6);
>  	memcpy(header->version, "00", 2);
>  
> -	xsnprintf(header->chksum, sizeof(header->chksum), "%07o", ustar_header_chksum(header));
> +	xsnprintf(header->chksum, sizeof(header->chksum), "%06o", ustar_header_chksum(header));
> +	header->chksum[6] = '\0';
> +	header->chksum[7] = ' ';

Wow. The choice of %07o is as old as the very original of tar-tree implementation in our codebase, starting at ae64bbc1 ("tar-tree: Introduce write_entry()", 2006-03-25).

I think the updated behaviour matches Wikipedia [*1*] where it spells out that 6 octal is followed by a NUL and a SP; it also says various implementations do not adhere to this format---perhaps they meant us ;-)

Frustratingly, I couldn't find this spelled out in POSIX [*2*] X-<. The closest I found there was

    All other fields are leading zero-filled octal numbers using
    digits from the ISO/IEC 646:1991 standard IRV. Each numeric
    field is terminated by one or more <space> or NUL characters.

which sounds as if the standard wanted to be deliberately more inclusive than what tar2sqfs is insisting on.

I think a change like this would impact kernel.org folks' tarball uploading tool, but that is not a reason not to apply this patch.

[References].

*1* https://en.wikipedia.org/wiki/Tar_(computing) *2* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/pax.html