{"thread":{"id":"51513","subject":"[PATCH] archive: Store checksum correctly","startedAt":"2019-07-23T02:58:06Z","lastAt":"2019-07-24T01:04:50Z","messageCount":8,"participants":["Matt Turner","Junio C Hamano","Jeff King","René Scharfe","David Oberhollenzer","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"379163","messageId":"20190723025736.23036-1-mattst88@gmail.com","threadId":"51513","inReplyTo":null,"subject":"[PATCH] archive: Store checksum correctly","fromName":"Matt Turner","fromEmail":"mattst88@gmail.com","sentAt":"2019-07-23T02:57:36Z","receivedAt":"2019-07-23T02:58:06Z","isPatch":true,"sender":{"key":"mattst88@gmail.com","avatar":null},"body":"tar2sqfs (part of https://github.com/topics/tar2sqfs) rejects tarballs\nmade with git archive with the message\n\n    invalid tar header checksum!\n\ntar2sqfs recomputes the tarball's checksum to verify it. Its checksum\nimplementation agrees with GNU tar, which contains a comment that states\n\n    Fill in the checksum field.  It's formatted differently from the\n    other fields: it has [6] digits, a null, then a space ...\n\nCorrecting this allows tar2sqfs to correctly process the tarballs made\nby git archive.\n\nSigned-off-by: Matt Turner <mattst88@gmail.com>\n---\n archive-tar.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 3e53aac1e6..f9a157bfd1 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -215,7 +215,9 @@ static void prepare_header(struct archiver_args *args,\n \tmemcpy(header->magic, \"ustar\", 6);\n \tmemcpy(header->version, \"00\", 2);\n \n-\txsnprintf(header->chksum, sizeof(header->chksum), \"%07o\", ustar_header_chksum(header));\n+\txsnprintf(header->chksum, sizeof(header->chksum), \"%06o\", ustar_header_chksum(header));\n+\theader->chksum[6] = '\\0';\n+\theader->chksum[7] = ' ';\n }\n \n static void write_extended_header(struct archiver_args *args,\n-- \n2.22.0.dirty\n\n"},{"id":"379173","messageId":"xmqqd0i0u253.fsf@gitster-ct.c.googlers.com","threadId":"51513","inReplyTo":"20190723025736.23036-1-mattst88@gmail.com","subject":"Re: [PATCH] archive: Store checksum correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-23T16:49:44Z","receivedAt":"2019-07-23T16:49:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt Turner <mattst88@gmail.com> writes:\n\n> tar2sqfs (part of https://github.com/topics/tar2sqfs) rejects tarballs\n> made with git archive with the message\n>\n>     invalid tar header checksum!\n>\n> tar2sqfs recomputes the tarball's checksum to verify it. Its checksum\n> implementation agrees with GNU tar, which contains a comment that states\n>\n>     Fill in the checksum field.  It's formatted differently from the\n>     other fields: it has [6] digits, a null, then a space ...\n>\n> Correcting this allows tar2sqfs to correctly process the tarballs made\n> by git archive.\n>\n> Signed-off-by: Matt Turner <mattst88@gmail.com>\n> ---\n>  archive-tar.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/archive-tar.c b/archive-tar.c\n> index 3e53aac1e6..f9a157bfd1 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -215,7 +215,9 @@ static void prepare_header(struct archiver_args *args,\n>  \tmemcpy(header->magic, \"ustar\", 6);\n>  \tmemcpy(header->version, \"00\", 2);\n>  \n> -\txsnprintf(header->chksum, sizeof(header->chksum), \"%07o\", ustar_header_chksum(header));\n> +\txsnprintf(header->chksum, sizeof(header->chksum), \"%06o\", ustar_header_chksum(header));\n> +\theader->chksum[6] = '\\0';\n> +\theader->chksum[7] = ' ';\n\nWow.  The choice of %07o is as old as the very original of tar-tree\nimplementation in our codebase, starting at ae64bbc1 (\"tar-tree:\nIntroduce write_entry()\", 2006-03-25).\n\nI think the updated behaviour matches Wikipedia [*1*] where it\nspells out that 6 octal is followed by a NUL and a SP; it also says\nvarious implementations do not adhere to this format---perhaps they\nmeant us ;-)\n\nFrustratingly, I couldn't find this spelled out in POSIX [*2*] X-<.\nThe closest I found there was\n\n    All other fields are leading zero-filled octal numbers using\n    digits from the ISO/IEC 646:1991 standard IRV. Each numeric\n    field is terminated by one or more <space> or NUL characters.\n\nwhich sounds as if the standard wanted to be deliberately more\ninclusive than what tar2sqfs is insisting on.\n\nI think a change like this would impact kernel.org folks' tarball\nuploading tool, but that is not a reason not to apply this patch.\n\n\n\n[References].\n\n*1* https://en.wikipedia.org/wiki/Tar_(computing)\n*2* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/pax.html\n\n"},{"id":"379178","messageId":"20190723193119.GA3879@sigill.intra.peff.net","threadId":"51513","inReplyTo":"xmqqd0i0u253.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] archive: Store checksum correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-23T19:31:19Z","receivedAt":"2019-07-23T19:31:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 23, 2019 at 09:49:44AM -0700, Junio C Hamano wrote:\n\n> I think a change like this would impact kernel.org folks' tarball\n> uploading tool, but that is not a reason not to apply this patch.\n\nThat was my thought, too. Distro projects like homebrew rely on\nstable hashes of upstream tarballs, and they get cranky when those\ntarballs change. Ideally the projects they package would provide\nbyte-stable tarballs, but many of them rely on on-the-fly tarball\ngeneration by hosting sites like GitHub.\n\nWhich isn't to say we should never fix bugs in the tarballs that we\nproduce, but it's not entirely clear to me that this _is_ a bug, and not\njust one tool being overly picky.\n\n-Peff\n"},{"id":"379179","messageId":"14410e00-4701-40d0-6960-e481fea50ed0@web.de","threadId":"51513","inReplyTo":"xmqqd0i0u253.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] archive: Store checksum correctly","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-07-23T19:38:47Z","receivedAt":"2019-07-23T19:39:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.07.19 um 18:49 schrieb Junio C Hamano:\n> Matt Turner <mattst88@gmail.com> writes:\n>\n>> tar2sqfs (part of https://github.com/topics/tar2sqfs) rejects tarballs\n>> made with git archive with the message\n>>\n>>     invalid tar header checksum!\n>>\n>> tar2sqfs recomputes the tarball's checksum to verify it. Its checksum\n>> implementation agrees with GNU tar, which contains a comment that states\n>>\n>>     Fill in the checksum field.  It's formatted differently from the\n>>     other fields: it has [6] digits, a null, then a space ...\n>>\n>> Correcting this allows tar2sqfs to correctly process the tarballs made\n>> by git archive.\n>>\n>> Signed-off-by: Matt Turner <mattst88@gmail.com>\n>> ---\n>>  archive-tar.c | 4 +++-\n>>  1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/archive-tar.c b/archive-tar.c\n>> index 3e53aac1e6..f9a157bfd1 100644\n>> --- a/archive-tar.c\n>> +++ b/archive-tar.c\n>> @@ -215,7 +215,9 @@ static void prepare_header(struct archiver_args *args,\n>>  \tmemcpy(header->magic, \"ustar\", 6);\n>>  \tmemcpy(header->version, \"00\", 2);\n>>\n>> -\txsnprintf(header->chksum, sizeof(header->chksum), \"%07o\", ustar_header_chksum(header));\n>> +\txsnprintf(header->chksum, sizeof(header->chksum), \"%06o\", ustar_header_chksum(header));\n>> +\theader->chksum[6] = '\\0';\n>> +\theader->chksum[7] = ' ';\n>\n> Wow.  The choice of %07o is as old as the very original of tar-tree\n> implementation in our codebase, starting at ae64bbc1 (\"tar-tree:\n> Introduce write_entry()\", 2006-03-25).\n\nActually it's already in 731ab9ccf2 (\"[PATCH] create tar archives of\ntree on the fly\", 2005-04-28).\n\n> I think the updated behaviour matches Wikipedia [*1*] where it\n> spells out that 6 octal is followed by a NUL and a SP; it also says\n> various implementations do not adhere to this format---perhaps they\n> meant us ;-)\n\nOpenBSD's pax(1) does the same if I read\nhttps://github.com/openbsd/src/blob/master/bin/pax/tar.c correctly.\n\n> Frustratingly, I couldn't find this spelled out in POSIX [*2*] X-<.\n> The closest I found there was\n>\n>     All other fields are leading zero-filled octal numbers using\n>     digits from the ISO/IEC 646:1991 standard IRV. Each numeric\n>     field is terminated by one or more <space> or NUL characters.\n>\n> which sounds as if the standard wanted to be deliberately more\n> inclusive than what tar2sqfs is insisting on.\n\nYes, I remember following that specific paragraph when writing the\ncode.  The wording probably tries to account for the different\nvariants invented by vendors over the decades.\n\nis_checksum_valid() in\nhttps://github.com/AgentD/squashfs-tools-ng/blob/master/lib/tar/checksum.c\ncompares the formatted checksum byte-by-byte.  That seems\nunnecessarily strict.  Parsing and comparing the numerical value\nof the checksum would be more forgiving, better adhere to POSIX and\nmight be a tiny bit quicker.\n\nRené\n"},{"id":"379185","messageId":"9a0c89e4-b216-86f1-c721-c54fb2ee0a10@web.de","threadId":"51513","inReplyTo":"14410e00-4701-40d0-6960-e481fea50ed0@web.de","subject":"Re: [PATCH] archive: Store checksum correctly","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-07-23T20:08:38Z","receivedAt":"2019-07-23T20:08:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.07.19 um 21:38 schrieb René Scharfe:\n> is_checksum_valid() in\n> https://github.com/AgentD/squashfs-tools-ng/blob/master/lib/tar/checksum.c\n> compares the formatted checksum byte-by-byte.  That seems\n> unnecessarily strict.  Parsing and comparing the numerical value\n> of the checksum would be more forgiving, better adhere to POSIX and\n> might be a tiny bit quicker.\n\nI mean something like the patch below.  Code and text size are bigger,\nbut it's more lenient and writes less.  Untested.\n\n(Side note: I'm a bit surprised that GCC 8.3 adds the eight spaces one\n by one in the middle loop with -O2..)\n\n---\n lib/tar/checksum.c | 31 ++++++++++++++++++++++---------\n 1 file changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/lib/tar/checksum.c b/lib/tar/checksum.c\nindex a2a101a..af94ab4 100644\n--- a/lib/tar/checksum.c\n+++ b/lib/tar/checksum.c\n@@ -1,15 +1,27 @@\n /* SPDX-License-Identifier: GPL-3.0-or-later */\n #include \"internal.h\"\n\n-void update_checksum(tar_header_t *hdr)\n+static unsigned int get_checksum(const tar_header_t *hdr)\n {\n+\tconst unsigned char *header_start = (const unsigned char *)hdr;\n+\tconst unsigned char *chksum_start = (const unsigned char *)hdr->chksum;\n+\tconst unsigned char *header_end = header_start + sizeof(*hdr);\n+\tconst unsigned char *chksum_end = chksum_start + sizeof(hdr->chksum);\n+\tconst unsigned char *p;\n \tunsigned int chksum = 0;\n-\tsize_t i;\n\n-\tmemset(hdr->chksum, ' ', sizeof(hdr->chksum));\n+\tfor (p = header_start; p < chksum_start; p++)\n+\t\tchksum += *p;\n+\tfor (; p < chksum_end; p++)\n+\t\tchksum += ' ';\n+\tfor (; p < header_end; p++)\n+\t\tchksum += *p;\n+\treturn chksum;\n+}\n\n-\tfor (i = 0; i < sizeof(*hdr); ++i)\n-\t\tchksum += ((unsigned char *)hdr)[i];\n+void update_checksum(tar_header_t *hdr)\n+{\n+\tunsigned int chksum = get_checksum(hdr);\n\n \tsprintf(hdr->chksum, \"%06o\", chksum);\n \thdr->chksum[6] = '\\0';\n@@ -18,9 +30,10 @@ void update_checksum(tar_header_t *hdr)\n\n bool is_checksum_valid(const tar_header_t *hdr)\n {\n-\ttar_header_t copy;\n+\tunsigned int calculated_chksum = get_checksum(hdr);\n+\tuint64_t read_chksum;\n\n-\tmemcpy(&copy, hdr, sizeof(*hdr));\n-\tupdate_checksum(&copy);\n-\treturn memcmp(hdr, &copy, sizeof(*hdr)) == 0;\n+\tif (read_octal(hdr->chksum, sizeof(hdr->chksum), &read_chksum))\n+\t\treturn 0;\n+\treturn read_chksum == calculated_chksum;\n }\n--\n2.22.0\n"},{"id":"379190","messageId":"xmqqef2gsb07.fsf@gitster-ct.c.googlers.com","threadId":"51513","inReplyTo":"14410e00-4701-40d0-6960-e481fea50ed0@web.de","subject":"Re: [PATCH] archive: Store checksum correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-23T21:21:12Z","receivedAt":"2019-07-23T21:21:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> Wow.  The choice of %07o is as old as the very original of tar-tree\n>> implementation in our codebase, starting at ae64bbc1 (\"tar-tree:\n>> Introduce write_entry()\", 2006-03-25).\n>\n> Actually it's already in 731ab9ccf2 (\"[PATCH] create tar archives of\n> tree on the fly\", 2005-04-28).\n\nYup, after viewing \"git show ae64bbc1\" I found out the commit added\na new helper to do %07o without touching the existing one that did\nthe same.  Problem with relying on \"git blame\" too much X-<.\n\n>> I think the updated behaviour matches Wikipedia [*1*] where it\n>> spells out that 6 octal is followed by a NUL and a SP; it also says\n>> various implementations do not adhere to this format---perhaps they\n>> meant us ;-)\n>\n> OpenBSD's pax(1) does the same if I read\n> https://github.com/openbsd/src/blob/master/bin/pax/tar.c correctly.\n\nWhat's more interesting is that their verifier in tar_id() compares\nthe ulong value read from textual checksum with the ulong value\ncomputed.  I agree with you that it would be the more robust way\nthan what is done by squshfs tools (ng).\n"},{"id":"379191","messageId":"25d47ba2-a22c-cafc-150d-c649d6aead9f@sigma-star.at","threadId":"51513","inReplyTo":"9a0c89e4-b216-86f1-c721-c54fb2ee0a10@web.de","subject":"Re: [PATCH] archive: Store checksum correctly","fromName":"David Oberhollenzer","fromEmail":"david.oberhollenzer@sigma-star.at","sentAt":"2019-07-23T21:34:19Z","receivedAt":"2019-07-23T21:34:33Z","isPatch":true,"sender":{"key":"david.oberhollenzer@sigma-star.at","avatar":null},"body":"Hi,\n\nthe implementation of tar2sqfs was primarily based on `man 5 tar`[0],\nWikipedia[1] and trial/error with archives generated by GNU tar and\nsamples from [2] which I also integrated as test cases.\n\nOn 7/23/19 10:08 PM, René Scharfe wrote:\n> Am 23.07.19 um 21:38 schrieb René Scharfe:\n>> is_checksum_valid() in\n>> https://github.com/AgentD/squashfs-tools-ng/blob/master/lib/tar/checksum.c\n>> compares the formatted checksum byte-by-byte.  That seems\n>> unnecessarily strict.  Parsing and comparing the numerical value\n>> of the checksum would be more forgiving, better adhere to POSIX and\n>> might be a tiny bit quicker.\n>\n\nI agree with that. The current code was probably the simplest way to\nmove forward with existing code after implementing sqfs2tar and it stayed\nthat way, since it worked with all archives I tested it with.\n\nAs for every file format parser, tar2sqfs should of course be as tolerant\nas possible in what it accepts.\n\n> I mean something like the patch below.  Code and text size are bigger,\n> but it's more lenient and writes less.  Untested.\n> \nI applied and tested the patch. It looks similar to the solution I came up\nwith and the test cases still pass. Also, on my system tar2sqfs now accepts\ngit-archive tar balls which it didn't before applying the patch, so I\nwould go with this.\n\nThanks,\n\nDavid\n\n\n[0] https://www.freebsd.org/cgi/man.cgi?query=tar&sektion=5&manpath=FreeBSD+12.0-RELEASE&arch=default&format=html\n[1] https://en.wikipedia.org/wiki/Tar_(computing)#File_format\n[2] https://dev.gentoo.org/~mgorny/articles/portability-of-tar-features.html\n"},{"id":"379192","messageId":"20190724010445.GA164908@google.com","threadId":"51513","inReplyTo":"20190723025736.23036-1-mattst88@gmail.com","subject":"Re: [PATCH] archive: Store checksum correctly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-07-24T01:04:45Z","receivedAt":"2019-07-24T01:04:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc: René Scharfe, \"git archive\" expert)\nMatt Turner wrote:\n\n> tar2sqfs (part of https://github.com/topics/tar2sqfs) rejects tarballs\n> made with git archive with the message\n>\n>     invalid tar header checksum!\n>\n> tar2sqfs recomputes the tarball's checksum to verify it. Its checksum\n> implementation agrees with GNU tar, which contains a comment that states\n>\n>     Fill in the checksum field.  It's formatted differently from the\n>     other fields: it has [6] digits, a null, then a space ...\n>\n> Correcting this allows tar2sqfs to correctly process the tarballs made\n> by git archive.\n>\n> Signed-off-by: Matt Turner <mattst88@gmail.com>\n> ---\n>  archive-tar.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n\nNice.  Is this something that can be covered in tests as well?  (See\nt500* for existing \"git archive\" tests, and see test_lazy_prereq in case\nyou'd like the test to use an external tool like tar2sqfs that not all\nusers may have.)\n\nThanks,\nJonathan\n\n(patch left unsnipped for reference)\n\n> diff --git a/archive-tar.c b/archive-tar.c\n> index 3e53aac1e6..f9a157bfd1 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -215,7 +215,9 @@ static void prepare_header(struct archiver_args *args,\n>  \tmemcpy(header->magic, \"ustar\", 6);\n>  \tmemcpy(header->version, \"00\", 2);\n>  \n> -\txsnprintf(header->chksum, sizeof(header->chksum), \"%07o\", ustar_header_chksum(header));\n> +\txsnprintf(header->chksum, sizeof(header->chksum), \"%06o\", ustar_header_chksum(header));\n> +\theader->chksum[6] = '\\0';\n> +\theader->chksum[7] = ' ';\n>  }\n>  \n>  static void write_extended_header(struct archiver_args *args,\n"}]}