{"thread":{"id":"51164","subject":"git archive generates tar with malformed pax extended attribute","startedAt":"2019-05-24T06:45:36Z","lastAt":"2019-06-04T20:53:51Z","messageCount":21,"participants":["Keegan Carruthers-Smith","Jeff King","René Scharfe","Andreas Schwab","Ævar Arnfjörð Bjarmason","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"376146","messageId":"CAMVcy0Q0TL6uEGR2NeudJrOiXdQ87XcducL0EwMidWucjk5XYw@mail.gmail.com","threadId":"51164","inReplyTo":null,"subject":"git archive generates tar with malformed pax extended attribute","fromName":"Keegan Carruthers-Smith","fromEmail":"keegan.csmith@gmail.com","sentAt":"2019-05-24T06:45:23Z","receivedAt":"2019-05-24T06:45:36Z","isPatch":false,"sender":{"key":"keegan.csmith@gmail.com","avatar":"https://gravatar.com/avatar/42716e751b8473bf2e823218d835b52507c5654a93561d1a2cd32a9a14a2293a?d=mp&s=160"},"body":"Hello,\n\ngit archive can generate a malformed tar archive. bsdtar reports the\nerror \"tar: Ignoring malformed pax extended attribute\" when reading\nthe archive. Go's \"tar/archive\" package also reports the error\n\"archive/tar: invalid tar header\". However, BusyBox's tar does not\nreport the error (unsure if it just has less error logging).\n\nI can reproduce this when generating the tar on linux and mac. I\ntested this with \"git version 2.21.0\" and a build of next I did today:\n\"git version 2.22.0.rc1.257.g3120a18244\"\n\nReproduction:\n\n  $ git clone https://github.com/SSW-SCIENTIFIC/NNDD.git\n  $ cd NNDD\n  $ git archive --format tar c21b98da2ca7f007230e696b2eda5da6589fe137\n| tar tf - > /dev/null\n  tar: Ignoring malformed pax extended attribute\n  tar: Error exit delayed from previous errors.\n\nKind Regards,\nKeegan\n"},{"id":"376148","messageId":"20190524070644.GF25694@sigill.intra.peff.net","threadId":"51164","inReplyTo":"CAMVcy0Q0TL6uEGR2NeudJrOiXdQ87XcducL0EwMidWucjk5XYw@mail.gmail.com","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-24T07:06:44Z","receivedAt":"2019-05-24T07:06:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 24, 2019 at 08:45:23AM +0200, Keegan Carruthers-Smith wrote:\n\n> git archive can generate a malformed tar archive. bsdtar reports the\n> error \"tar: Ignoring malformed pax extended attribute\" when reading\n> the archive. Go's \"tar/archive\" package also reports the error\n> \"archive/tar: invalid tar header\". However, BusyBox's tar does not\n> report the error (unsure if it just has less error logging).\n> \n> I can reproduce this when generating the tar on linux and mac. I\n> tested this with \"git version 2.21.0\" and a build of next I did today:\n> \"git version 2.22.0.rc1.257.g3120a18244\"\n> \n> Reproduction:\n> \n>   $ git clone https://github.com/SSW-SCIENTIFIC/NNDD.git\n>   $ cd NNDD\n>   $ git archive --format tar c21b98da2ca7f007230e696b2eda5da6589fe137\n> | tar tf - > /dev/null\n>   tar: Ignoring malformed pax extended attribute\n>   tar: Error exit delayed from previous errors.\n\nI can't reproduce on Linux, using GNU tar (1.30) nor with bsdtar 3.3.3\n(from Debian's bsdtar package). What does your \"tar --version\" say?\n\nGit does write a pax header with the commit id in it as a comment.\nPresumably that's what it's complaining about (but it is not malformed\naccording to any tar I've tried). If you feed git-archive a tree rather\nthan a commit, that is omitted. What does:\n\n  git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n\nsay? If it doesn't complain, then we know it's indeed the pax comment\nfield.\n\n-Peff\n"},{"id":"376151","messageId":"CAMVcy0ThtcDNjqat0+nQ4B91hC30NTUe=RW8v9WDxA2Q-4SyRA@mail.gmail.com","threadId":"51164","inReplyTo":"20190524070644.GF25694@sigill.intra.peff.net","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Keegan Carruthers-Smith","fromEmail":"keegan.csmith@gmail.com","sentAt":"2019-05-24T07:35:51Z","receivedAt":"2019-05-24T07:36:05Z","isPatch":false,"sender":{"key":"keegan.csmith@gmail.com","avatar":"https://gravatar.com/avatar/42716e751b8473bf2e823218d835b52507c5654a93561d1a2cd32a9a14a2293a?d=mp&s=160"},"body":"On Fri, 24 May 2019 at 09:06, Jeff King <peff@peff.net> wrote:\n>\n> On Fri, May 24, 2019 at 08:45:23AM +0200, Keegan Carruthers-Smith wrote:\n>\n> > git archive can generate a malformed tar archive. bsdtar reports the\n> > error \"tar: Ignoring malformed pax extended attribute\" when reading\n> > the archive. Go's \"tar/archive\" package also reports the error\n> > \"archive/tar: invalid tar header\". However, BusyBox's tar does not\n> > report the error (unsure if it just has less error logging).\n> >\n> > I can reproduce this when generating the tar on linux and mac. I\n> > tested this with \"git version 2.21.0\" and a build of next I did today:\n> > \"git version 2.22.0.rc1.257.g3120a18244\"\n> >\n> > Reproduction:\n> >\n> >   $ git clone https://github.com/SSW-SCIENTIFIC/NNDD.git\n> >   $ cd NNDD\n> >   $ git archive --format tar c21b98da2ca7f007230e696b2eda5da6589fe137\n> > | tar tf - > /dev/null\n> >   tar: Ignoring malformed pax extended attribute\n> >   tar: Error exit delayed from previous errors.\n>\n> I can't reproduce on Linux, using GNU tar (1.30) nor with bsdtar 3.3.3\n> (from Debian's bsdtar package). What does your \"tar --version\" say?\n\nbsdtar 2.8.3 - libarchive 2.8.3\n\n> Git does write a pax header with the commit id in it as a comment.\n> Presumably that's what it's complaining about (but it is not malformed\n> according to any tar I've tried). If you feed git-archive a tree rather\n> than a commit, that is omitted. What does:\n>\n>   git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n>\n> say? If it doesn't complain, then we know it's indeed the pax comment\n> field.\n\nIt also complains\n\n  $ git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n  tar: Ignoring malformed pax extended attribute\n  tar: Error exit delayed from previous errors.\n\nSome more context: I work at Sourcegraph.com We mirror a lot of repos\nfrom github.com. We usually interact with a working copy by running\ngit archive on it in our infrastructure. This is the first repository\nthat I have noticed which produces this error. An interesting thing to\nnote is the commit metadata contains a lot of non-ascii text which was\nmy guess at what my be tripping up the tar creation.\n\nKeegan\n"},{"id":"376154","messageId":"20190524081337.GA9082@sigill.intra.peff.net","threadId":"51164","inReplyTo":"CAMVcy0ThtcDNjqat0+nQ4B91hC30NTUe=RW8v9WDxA2Q-4SyRA@mail.gmail.com","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-24T08:13:37Z","receivedAt":"2019-05-24T08:13:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 24, 2019 at 09:35:51AM +0200, Keegan Carruthers-Smith wrote:\n\n> > I can't reproduce on Linux, using GNU tar (1.30) nor with bsdtar 3.3.3\n> > (from Debian's bsdtar package). What does your \"tar --version\" say?\n> \n> bsdtar 2.8.3 - libarchive 2.8.3\n\nInteresting. I wonder if there was a libarchive bug that was fixed\nbetween 2.8.3 and 3.3.3.\n\n> > Git does write a pax header with the commit id in it as a comment.\n> > Presumably that's what it's complaining about (but it is not malformed\n> > according to any tar I've tried). If you feed git-archive a tree rather\n> > than a commit, that is omitted. What does:\n> >\n> >   git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n> >\n> > say? If it doesn't complain, then we know it's indeed the pax comment\n> > field.\n> \n> It also complains\n> \n>   $ git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n>   tar: Ignoring malformed pax extended attribute\n>   tar: Error exit delayed from previous errors.\n\nAh, OK. So it's not the comment field at all, but some other entry.\n\n> Some more context: I work at Sourcegraph.com We mirror a lot of repos\n> from github.com. We usually interact with a working copy by running\n> git archive on it in our infrastructure. This is the first repository\n> that I have noticed which produces this error. An interesting thing to\n> note is the commit metadata contains a lot of non-ascii text which was\n> my guess at what my be tripping up the tar creation.\n\nYeah, though the only thing that makes it into the tarfile is the actual\ntree entries. I'd imagine the file content is not likely to be a source\nof problems, as it's common to see binary gunk there. Most of the\nfilenames are pretty mundane, but this symlink destination is a little\nfunny:\n\n  $ git archive ... | tar tvf - | grep nicovideo4as.swc\n  lrwxrwxrwx root/root         0 2019-05-24 03:05 libs/nicovideo4as.swc -> PK\\003\\004\\024\n\nThat's not the full story, though. It is indeed a symlink in the\ntree:\n\n  $ git ls-tree -r HEAD libs/nicovideo4as.swc\n  120000 blob ec3137b5fcaeae25cf67927068af116517683806\tlibs/nicovideo4as.swc\n\nBut the contents of that blob, which should be the destination filename,\nare definitely not:\n\n  $ git cat-file blob ec3137b5f | wc -c\n  57804\n  $ git cat-file blob ec3137b5f | xxd | head -1\n  00000000: 504b 0304 1400 0800 0800 5069 694e 0000  PK........PiiN..\n\nThere's quite a bit more data there. And what tar showed us goes up to\nthe first NUL, which does not seem surprising.\n\nIt's possible Git is doing the wrong thing on the writing side, but\ngiven that newer versions of bsdtar handle it fine, I'd guess that the\nold one simply had problems consuming poorly formed symlink filenames.\n\n-Peff\n"},{"id":"376216","messageId":"b4aaff4b-eaf7-9eaf-063f-42c073078060@web.de","threadId":"51164","inReplyTo":"20190524081337.GA9082@sigill.intra.peff.net","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-05-25T13:26:53Z","receivedAt":"2019-05-25T13:27:18Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.05.19 um 10:13 schrieb Jeff King:\n> On Fri, May 24, 2019 at 09:35:51AM +0200, Keegan Carruthers-Smith wrote:\n>\n>>> I can't reproduce on Linux, using GNU tar (1.30) nor with bsdtar 3.3.3\n>>> (from Debian's bsdtar package). What does your \"tar --version\" say?\n>>\n>> bsdtar 2.8.3 - libarchive 2.8.3\n>\n> Interesting. I wonder if there was a libarchive bug that was fixed\n> between 2.8.3 and 3.3.3.\n>\n>>> Git does write a pax header with the commit id in it as a comment.\n>>> Presumably that's what it's complaining about (but it is not malformed\n>>> according to any tar I've tried). If you feed git-archive a tree rather\n>>> than a commit, that is omitted. What does:\n>>>\n>>>   git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n>>>\n>>> say? If it doesn't complain, then we know it's indeed the pax comment\n>>> field.\n>>\n>> It also complains\n>>\n>>   $ git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n>>   tar: Ignoring malformed pax extended attribute\n>>   tar: Error exit delayed from previous errors.\n>\n> Ah, OK. So it's not the comment field at all, but some other entry.\n>\n>> Some more context: I work at Sourcegraph.com We mirror a lot of repos\n>> from github.com. We usually interact with a working copy by running\n>> git archive on it in our infrastructure. This is the first repository\n>> that I have noticed which produces this error. An interesting thing to\n>> note is the commit metadata contains a lot of non-ascii text which was\n>> my guess at what my be tripping up the tar creation.\n>\n> Yeah, though the only thing that makes it into the tarfile is the actual\n> tree entries. I'd imagine the file content is not likely to be a source\n> of problems, as it's common to see binary gunk there. Most of the\n> filenames are pretty mundane, but this symlink destination is a little\n> funny:\n>\n>   $ git archive ... | tar tvf - | grep nicovideo4as.swc\n>   lrwxrwxrwx root/root         0 2019-05-24 03:05 libs/nicovideo4as.swc -> PK\\003\\004\\024\n>\n> That's not the full story, though. It is indeed a symlink in the\n> tree:\n>\n>   $ git ls-tree -r HEAD libs/nicovideo4as.swc\n>   120000 blob ec3137b5fcaeae25cf67927068af116517683806\tlibs/nicovideo4as.swc\n>\n> But the contents of that blob, which should be the destination filename,\n> are definitely not:\n>\n>   $ git cat-file blob ec3137b5f | wc -c\n>   57804\n>   $ git cat-file blob ec3137b5f | xxd | head -1\n>   00000000: 504b 0304 1400 0800 0800 5069 694e 0000  PK........PiiN..\n>\n> There's quite a bit more data there. And what tar showed us goes up to\n> the first NUL, which does not seem surprising.\n\nThat (the symlink target) is a ZIP file with the following contents:\n\n Length   Method    Size  Cmpr    Date    Time   CRC-32   Name\n--------  ------  ------- ---- ---------- ----- --------  ----\n   39733  Defl:N     3403  91% 2019-03-09 13:10 489e1be1  catalog.xml\n   54131  Defl:N    54151   0% 2019-03-09 13:10 32f57322  library.swf\n--------          -------  ---                            -------\n   93864            57554  39%                            2 files\n\nAnd link targets longer than 100 characters are encoded in an extended\nPax header.\n\n(Usually symlink targets are paths, not file contents.)\n\n> It's possible Git is doing the wrong thing on the writing side, but\n> given that newer versions of bsdtar handle it fine, I'd guess that the\n> old one simply had problems consuming poorly formed symlink filenames.\n\nGit preserves symlink targets with embedded NULs in the repository and\nin generated tar files.  Not sure if GNU tar and bsdtar truncating them\nat the first NUL is a bug.  I'm also not sure if there is a platform\nthat would allow creating such a symlink in the file system, or how one\nis supposed to use it.\n\nWe could truncate symlink targets at the first NUL as well in git\narchive -- but that would be a bit sad, as the archive formats allow\nstoring the \"real\" target from the repo, with NUL and all.  We could\nmake git fsck report such symlinks.\n\nCan Unicode symlink targets contain NULs?  We wouldn't want to damage\nthem even if we decide to truncate.\n\nRené\n"},{"id":"376217","messageId":"87tvdi3aw2.fsf@igel.home","threadId":"51164","inReplyTo":"b4aaff4b-eaf7-9eaf-063f-42c073078060@web.de","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2019-05-25T13:46:05Z","receivedAt":"2019-05-25T13:46:10Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Mai 25 2019, René Scharfe <l.s.r@web.de> wrote:\n\n> Can Unicode symlink targets contain NULs?  We wouldn't want to damage\n> them even if we decide to truncate.\n\nThe POSIX interface doesn't allow creating such a symlink, since the\nargument to symlink is a C string.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"376221","messageId":"878suufejr.fsf@evledraar.gmail.com","threadId":"51164","inReplyTo":"20190524070644.GF25694@sigill.intra.peff.net","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-25T20:46:16Z","receivedAt":"2019-05-25T20:46:22Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 24 2019, Jeff King wrote:\n\n> On Fri, May 24, 2019 at 08:45:23AM +0200, Keegan Carruthers-Smith wrote:\n>\n>> git archive can generate a malformed tar archive. bsdtar reports the\n>> error \"tar: Ignoring malformed pax extended attribute\" when reading\n>> the archive. Go's \"tar/archive\" package also reports the error\n>> \"archive/tar: invalid tar header\". However, BusyBox's tar does not\n>> report the error (unsure if it just has less error logging).\n>>\n>> I can reproduce this when generating the tar on linux and mac. I\n>> tested this with \"git version 2.21.0\" and a build of next I did today:\n>> \"git version 2.22.0.rc1.257.g3120a18244\"\n>>\n>> Reproduction:\n>>\n>>   $ git clone https://github.com/SSW-SCIENTIFIC/NNDD.git\n>>   $ cd NNDD\n>>   $ git archive --format tar c21b98da2ca7f007230e696b2eda5da6589fe137\n>> | tar tf - > /dev/null\n>>   tar: Ignoring malformed pax extended attribute\n>>   tar: Error exit delayed from previous errors.\n>\n> I can't reproduce on Linux, using GNU tar (1.30) nor with bsdtar 3.3.3\n> (from Debian's bsdtar package). What does your \"tar --version\" say?\n>\n> Git does write a pax header with the commit id in it as a comment.\n> Presumably that's what it's complaining about (but it is not malformed\n> according to any tar I've tried). If you feed git-archive a tree rather\n> than a commit, that is omitted. What does:\n>\n>   git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n>\n> say? If it doesn't complain, then we know it's indeed the pax comment\n> field.\n\nSolaris tar also complains about this. I've seen that for ages, but\nnever thought to report it, I figured it was well-known.\n\nWhen you \"tar xf\" an archive git-archive it complains:\n\n    tar: pax_global_header: typeflag 'g' not recognized, converting to regular file\n\nIt will then extract the \"pax_global_header\" as if it were a file at the\nroot of the archive. That file will look like this:\n\n    $ wc -c x/pax_global_header\n      52 x/pax_global_header\n    $ cat x/pax_global_header\n    52 comment=$40_CHAR_SHA_1\n\nWhere $40_CHAR_SHA_1 is whatever commit this archive was produced from.\n"},{"id":"376223","messageId":"877eaefdkt.fsf@evledraar.gmail.com","threadId":"51164","inReplyTo":"b4aaff4b-eaf7-9eaf-063f-42c073078060@web.de","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-25T21:07:14Z","receivedAt":"2019-05-25T21:07:22Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, May 25 2019, René Scharfe wrote:\n\n> Am 24.05.19 um 10:13 schrieb Jeff King:\n>> On Fri, May 24, 2019 at 09:35:51AM +0200, Keegan Carruthers-Smith wrote:\n>>\n>>>> I can't reproduce on Linux, using GNU tar (1.30) nor with bsdtar 3.3.3\n>>>> (from Debian's bsdtar package). What does your \"tar --version\" say?\n>>>\n>>> bsdtar 2.8.3 - libarchive 2.8.3\n>>\n>> Interesting. I wonder if there was a libarchive bug that was fixed\n>> between 2.8.3 and 3.3.3.\n>>\n>>>> Git does write a pax header with the commit id in it as a comment.\n>>>> Presumably that's what it's complaining about (but it is not malformed\n>>>> according to any tar I've tried). If you feed git-archive a tree rather\n>>>> than a commit, that is omitted. What does:\n>>>>\n>>>>   git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n>>>>\n>>>> say? If it doesn't complain, then we know it's indeed the pax comment\n>>>> field.\n>>>\n>>> It also complains\n>>>\n>>>   $ git archive --format tar c21b98da2^{tree} | tar tf - >/dev/null\n>>>   tar: Ignoring malformed pax extended attribute\n>>>   tar: Error exit delayed from previous errors.\n>>\n>> Ah, OK. So it's not the comment field at all, but some other entry.\n>>\n>>> Some more context: I work at Sourcegraph.com We mirror a lot of repos\n>>> from github.com. We usually interact with a working copy by running\n>>> git archive on it in our infrastructure. This is the first repository\n>>> that I have noticed which produces this error. An interesting thing to\n>>> note is the commit metadata contains a lot of non-ascii text which was\n>>> my guess at what my be tripping up the tar creation.\n>>\n>> Yeah, though the only thing that makes it into the tarfile is the actual\n>> tree entries. I'd imagine the file content is not likely to be a source\n>> of problems, as it's common to see binary gunk there. Most of the\n>> filenames are pretty mundane, but this symlink destination is a little\n>> funny:\n>>\n>>   $ git archive ... | tar tvf - | grep nicovideo4as.swc\n>>   lrwxrwxrwx root/root         0 2019-05-24 03:05 libs/nicovideo4as.swc -> PK\\003\\004\\024\n>>\n>> That's not the full story, though. It is indeed a symlink in the\n>> tree:\n>>\n>>   $ git ls-tree -r HEAD libs/nicovideo4as.swc\n>>   120000 blob ec3137b5fcaeae25cf67927068af116517683806\tlibs/nicovideo4as.swc\n>>\n>> But the contents of that blob, which should be the destination filename,\n>> are definitely not:\n>>\n>>   $ git cat-file blob ec3137b5f | wc -c\n>>   57804\n>>   $ git cat-file blob ec3137b5f | xxd | head -1\n>>   00000000: 504b 0304 1400 0800 0800 5069 694e 0000  PK........PiiN..\n>>\n>> There's quite a bit more data there. And what tar showed us goes up to\n>> the first NUL, which does not seem surprising.\n>\n> That (the symlink target) is a ZIP file with the following contents:\n>\n>  Length   Method    Size  Cmpr    Date    Time   CRC-32   Name\n> --------  ------  ------- ---- ---------- ----- --------  ----\n>    39733  Defl:N     3403  91% 2019-03-09 13:10 489e1be1  catalog.xml\n>    54131  Defl:N    54151   0% 2019-03-09 13:10 32f57322  library.swf\n> --------          -------  ---                            -------\n>    93864            57554  39%                            2 files\n>\n> And link targets longer than 100 characters are encoded in an extended\n> Pax header.\n>\n> (Usually symlink targets are paths, not file contents.)\n>\n>> It's possible Git is doing the wrong thing on the writing side, but\n>> given that newer versions of bsdtar handle it fine, I'd guess that the\n>> old one simply had problems consuming poorly formed symlink filenames.\n>\n> Git preserves symlink targets with embedded NULs in the repository and\n> in generated tar files.  Not sure if GNU tar and bsdtar truncating them\n> at the first NUL is a bug.  I'm also not sure if there is a platform\n> that would allow creating such a symlink in the file system, or how one\n> is supposed to use it.\n>\n> We could truncate symlink targets at the first NUL as well in git\n> archive -- but that would be a bit sad, as the archive formats allow\n> storing the \"real\" target from the repo, with NUL and all.  We could\n> make git fsck report such symlinks.\n>\n> Can Unicode symlink targets contain NULs?  We wouldn't want to damage\n> them even if we decide to truncate.\n\nI don't see a practical use for this case, and maybe we should even fsck\ncheck for the blob representing the symlink target having a \\0 in it as\nsuggested upthread.\n\nBut that being said, this assumption that data in a tar archive will get\nwritten to a FS of some sort isn't true. There's plenty of consumers of\nthe format that read it in-memory and stream its contents out to\nsomething else entirely, e.g. taking \"git archive --remote\" output,\nparsing it with e.g. [1] and throwing some/all of the content into a\ndatabase.\n\n1. https://metacpan.org/pod/Archive::Tar\n"},{"id":"376224","messageId":"20190525211942.GE8616@genre.crustytoothpaste.net","threadId":"51164","inReplyTo":"878suufejr.fsf@evledraar.gmail.com","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-25T21:19:42Z","receivedAt":"2019-05-25T21:19:51Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-05-25 at 20:46:16, Ævar Arnfjörð Bjarmason wrote:\n> Solaris tar also complains about this. I've seen that for ages, but\n> never thought to report it, I figured it was well-known.\n> \n> When you \"tar xf\" an archive git-archive it complains:\n> \n>     tar: pax_global_header: typeflag 'g' not recognized, converting to regular file\n> \n> It will then extract the \"pax_global_header\" as if it were a file at the\n> root of the archive. That file will look like this:\n> \n>     $ wc -c x/pax_global_header\n>       52 x/pax_global_header\n>     $ cat x/pax_global_header\n>     52 comment=$40_CHAR_SHA_1\n> \n> Where $40_CHAR_SHA_1 is whatever commit this archive was produced from.\n\nIronically, for a long time the pax(1) utility shipped with Debian was\nincapable of reading pax headers. There are various other utilities\n(including 7-Zip) that don't understand them, and they invariably get\nconverted to regular files (since that's what the spec says to do). This\nproblem is well known to me, but I don't know if it's well known to\neveryone.\n\nIn this case, I suspect Solaris has a pax(1) utility that works fine,\nbut nobody bothered to port that code to tar(1).\n\nUsually the lack of support is because people prefer GNU tar archives\nover actual POSIX tar and pax archives, so real-world archivers don't\nsee POSIX archives and don't handle the extensions properly.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"376231","messageId":"2b52ca7b-bb72-193a-9d6b-83003ce49424@web.de","threadId":"51164","inReplyTo":"877eaefdkt.fsf@evledraar.gmail.com","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-05-26T21:33:20Z","receivedAt":"2019-05-26T21:34:04Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.05.19 um 23:07 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, May 25 2019, René Scharfe wrote:\n>\n>> We could truncate symlink targets at the first NUL as well in git\n>> archive -- but that would be a bit sad, as the archive formats allow\n>> storing the \"real\" target from the repo, with NUL and all.\n\n> But that being said, this assumption that data in a tar archive will get\n> written to a FS of some sort isn't true. There's plenty of consumers of\n> the format that read it in-memory and stream its contents out to\n> something else entirely, e.g. taking \"git archive --remote\" output,\n> parsing it with e.g. [1] and throwing some/all of the content into a\n> database.\n>\n> 1. https://metacpan.org/pod/Archive::Tar\n\nGit archive writes link targets that are 100 characters long or less\ninto the appropriate field in the plain tar header.  It copies\neverything, including NULs, but unlike a PAX extended header that field\nlacks a length indicator, so extractors basically have to treat it as\nNUL-terminated.\n\nIf we want to preserve NUL in short link targets as well, we'd have to\nput such names into an PAX extended header..\n\n---\n archive-tar.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 3e53aac1e6..e8f55578d1 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -291,7 +291,8 @@ static int write_tar_entry(struct archiver_args *args,\n \t}\n\n \tif (S_ISLNK(mode)) {\n-\t\tif (size > sizeof(header.linkname)) {\n+\t\tif (size > sizeof(header.linkname) ||\n+\t\t    memchr(buffer, '\\0', size)) {\n \t\t\txsnprintf(header.linkname, sizeof(header.linkname),\n \t\t\t\t  \"see %s.paxheader\", oid_to_hex(oid));\n \t\t\tstrbuf_append_ext_header(&ext_header, \"linkpath\",\n--\n2.21.0\n"},{"id":"376233","messageId":"CAMVcy0TgmWPccokMeUmaPSkj=6QhZfaACEEqoSqqqL4g3WSwSA@mail.gmail.com","threadId":"51164","inReplyTo":"20190524081337.GA9082@sigill.intra.peff.net","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Keegan Carruthers-Smith","fromEmail":"keegan.csmith@gmail.com","sentAt":"2019-05-27T05:11:34Z","receivedAt":"2019-05-27T05:11:48Z","isPatch":false,"sender":{"key":"keegan.csmith@gmail.com","avatar":"https://gravatar.com/avatar/42716e751b8473bf2e823218d835b52507c5654a93561d1a2cd32a9a14a2293a?d=mp&s=160"},"body":"On Fri, 24 May 2019 at 10:13, Jeff King <peff@peff.net> wrote:\n\n> It's possible Git is doing the wrong thing on the writing side, but\n> given that newer versions of bsdtar handle it fine, I'd guess that the\n> old one simply had problems consuming poorly formed symlink filenames.\n\nI agree that the reader should be able to handle it best-effort.\nEspecially given both bsdtar and gnutar seem to not choke on it. I\nmentioned go's archive/tar chokes on it, so reported that issue\nhttps://github.com/golang/go/issues/32264\n\n-Keegan\n"},{"id":"376268","messageId":"20190528054434.GA7946@sigill.intra.peff.net","threadId":"51164","inReplyTo":"2b52ca7b-bb72-193a-9d6b-83003ce49424@web.de","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-28T05:44:35Z","receivedAt":"2019-05-28T05:44:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 26, 2019 at 11:33:20PM +0200, René Scharfe wrote:\n\n> Git archive writes link targets that are 100 characters long or less\n> into the appropriate field in the plain tar header.  It copies\n> everything, including NULs, but unlike a PAX extended header that field\n> lacks a length indicator, so extractors basically have to treat it as\n> NUL-terminated.\n> \n> If we want to preserve NUL in short link targets as well, we'd have to\n> put such names into an PAX extended header..\n\nI can't say that I care much either way, since putting NULs in your\nfilenames is pretty crazy. But that would at least make things\nconsistent regardless of the length.\n\nI also wondered if you could play any security tricks here (e.g., by\nhaving one tool view a filename as \"foo\" and another as \"foo\\0bar\"). I\ndoubt it, though.  Filenames themselves in Git are always\nNUL-terminated, since that's dictated the tree format; so this is really\njust about link destinations.\n\nAnd even then, it's hard to imagine a case. I was wondering if you could\nhave an entry \".git\\0foo\" that ends up written to the filesystem as\n\".git\". But these are tar archives we're talking about, so it's not like\nyou couldn't just put \".git\" in a tar file in the first place.\n\nBut maybe somebody else can brainstorm something more evil. :)\n\n-Peff\n"},{"id":"376269","messageId":"20190528055805.GB7946@sigill.intra.peff.net","threadId":"51164","inReplyTo":"b4aaff4b-eaf7-9eaf-063f-42c073078060@web.de","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-28T05:58:06Z","receivedAt":"2019-05-28T05:58:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 25, 2019 at 03:26:53PM +0200, René Scharfe wrote:\n\n> We could truncate symlink targets at the first NUL as well in git\n> archive -- but that would be a bit sad, as the archive formats allow\n> storing the \"real\" target from the repo, with NUL and all.  We could\n> make git fsck report such symlinks.\n\nThis is a little tricky, because fsck generally looks at individual\nobjects, and the bad pattern is a combination of a tree and a blob\ntogether. I think you could make it work by reusing some of the code and\npatterns from 9e84a6d758 (Merge branch 'jk/submodule-fsck-loose' into\nmaint, 2018-05-22).\n\n> Can Unicode symlink targets contain NULs?  We wouldn't want to damage\n> them even if we decide to truncate.\n\nOn Windows, I suppose, where pathnames can be UTF-16? I don't know how\nany of that works with Git. I guess we'd always have to assume the\nfilenames in Git are UTF-8 or at least some ASCII-superset, since we\ncannot encode NULs; and presumably that would extend to link\ndestinations, too. So I doubt it's a problem in practice. Personally,\nI'd wait until somebody with such a system cares enough to suggest a new\nbehavior, rather than trying to guess. :)\n\nLikewise, I think at this point with Keegan's original report that Git\nis doing something reasonable with a lousy input. Unless something\ninteresting comes out of the golang/go bug report discussion (thank you\nfor opening that!), it's probably not worth chasing hypotheticals.\n\n-Peff\n"},{"id":"376330","messageId":"5b312f6c-3375-66ba-efc7-931e4ddc4b11@web.de","threadId":"51164","inReplyTo":"20190528055805.GB7946@sigill.intra.peff.net","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-05-28T18:01:43Z","receivedAt":"2019-05-28T18:01:51Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.05.19 um 07:58 schrieb Jeff King:\n> On Sat, May 25, 2019 at 03:26:53PM +0200, René Scharfe wrote:\n>\n>> We could\n>> make git fsck report such symlinks.\n>\n> This is a little tricky, because fsck generally looks at individual\n> objects, and the bad pattern is a combination of a tree and a blob\n> together. I think you could make it work by reusing some of the code and\n> patterns from 9e84a6d758 (Merge branch 'jk/submodule-fsck-loose' into\n> maint, 2018-05-22).\n\nActually it's super easy, barely an inconvenience (SCNR, watched a lot\nof those rants recently)..  Did I miss something?\n\n-- >8 --\nSubject: [PATCH] fsck: warn about symlink targets containing NUL\n\nSymlink targets are kept as blob objects in Git repositories and can\nthus contain arbitrary characters.  They are truncated silently at the\nfirst NUL by symlink(2) during checkout, though.  Make git fsck warn\nabout such tree entries.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n fsck.c          | 32 ++++++++++++++++++++++++++++++++\n t/t1450-fsck.sh | 13 +++++++++++++\n 2 files changed, 45 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex 4703f55561..d434f5d75c 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -49,6 +49,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(MISSING_PARENT, ERROR) \\\n \tFUNC(MISSING_SPACE_BEFORE_DATE, ERROR) \\\n \tFUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \\\n+\tFUNC(MISSING_SYMLINK_OBJECT, ERROR) \\\n \tFUNC(MISSING_TAG, ERROR) \\\n \tFUNC(MISSING_TAG_ENTRY, ERROR) \\\n \tFUNC(MISSING_TAG_OBJECT, ERROR) \\\n@@ -58,6 +59,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(MISSING_TYPE_ENTRY, ERROR) \\\n \tFUNC(MULTIPLE_AUTHORS, ERROR) \\\n \tFUNC(TAG_OBJECT_NOT_TAG, ERROR) \\\n+\tFUNC(SYMLINK_OBJECT_NOT_BLOB, ERROR) \\\n \tFUNC(TREE_NOT_SORTED, ERROR) \\\n \tFUNC(UNKNOWN_TYPE, ERROR) \\\n \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n@@ -78,6 +80,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(NULL_SHA1, WARN) \\\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n+\tFUNC(NUL_IN_SYMLINK_TARGET, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n@@ -578,6 +581,33 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n\n+static int fsck_symlink(struct tree *tree, const char *name,\n+\t\t\tconst struct object_id *oid,\n+\t\t\tstruct fsck_options *options)\n+{\n+\tint ret = 0;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tvoid *buffer = read_object_file(oid, &type, &size);\n+\n+\tif (!buffer)\n+\t\tret = report(options, &tree->object,\n+\t\t\t     FSCK_MSG_MISSING_SYMLINK_OBJECT,\n+\t\t\t     \"cannot read blob object for symlink %s\", name);\n+\telse if (type != OBJ_BLOB)\n+\t\tret = report(options, &tree->object,\n+\t\t\t     FSCK_MSG_SYMLINK_OBJECT_NOT_BLOB,\n+\t\t\t     \"expected blob got %s for symlink %s\",\n+\t\t\t     type_name(type), name);\n+\telse if (memchr(buffer, '\\0', size))\n+\t\tret = report(options, &tree->object,\n+\t\t\t     FSCK_MSG_NUL_IN_SYMLINK_TARGET,\n+\t\t\t     \"NUL in target of symlink %s\", name);\n+\n+\tfree(buffer);\n+\treturn ret;\n+}\n+\n static int fsck_tree(struct tree *item, struct fsck_options *options)\n {\n \tint retval = 0;\n@@ -626,6 +656,8 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n+\t\tif (S_ISLNK(mode))\n+\t\t\tretval += fsck_symlink(item, name, oid, options);\n\n \t\tif (update_tree_entry_gently(&desc)) {\n \t\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 0f268a3664..ce9501d063 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -822,4 +822,17 @@ test_expect_success 'detect corrupt index file in fsck' '\n \ttest_i18ngrep \"bad index file\" errors\n '\n\n+test_expect_success 'detect NUL in symlink target' '\n+\ttest_when_finished \"git update-ref -d refs/heads/nul_in_symlink\" &&\n+\ttest_when_finished \"remove_object \\$commit\" &&\n+\ttest_when_finished \"remove_object \\$tree\" &&\n+\ttest_when_finished \"remove_object \\$blob\" &&\n+\tblob=$(echo fooQbar | q_to_nul | git hash-object -w --stdin) &&\n+\ttree=$(echo \"120000 blob $blob\tsymlink\" | git mktree) &&\n+\tcommit=$(git commit-tree $tree) &&\n+\tgit update-ref refs/heads/nul_in_symlink $commit &&\n+\tgit fsck 2>out &&\n+\ttest_i18ngrep \"NUL in target of symlink\" out\n+'\n+\n test_done\n--\n2.21.0\n"},{"id":"376342","messageId":"20190528190815.GA20499@sigill.intra.peff.net","threadId":"51164","inReplyTo":"5b312f6c-3375-66ba-efc7-931e4ddc4b11@web.de","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-28T19:08:15Z","receivedAt":"2019-05-28T19:08:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 28, 2019 at 08:01:43PM +0200, René Scharfe wrote:\n\n> Am 28.05.19 um 07:58 schrieb Jeff King:\n> > On Sat, May 25, 2019 at 03:26:53PM +0200, René Scharfe wrote:\n> >\n> >> We could\n> >> make git fsck report such symlinks.\n> >\n> > This is a little tricky, because fsck generally looks at individual\n> > objects, and the bad pattern is a combination of a tree and a blob\n> > together. I think you could make it work by reusing some of the code and\n> > patterns from 9e84a6d758 (Merge branch 'jk/submodule-fsck-loose' into\n> > maint, 2018-05-22).\n> \n> Actually it's super easy, barely an inconvenience (SCNR, watched a lot\n> of those rants recently)..  Did I miss something?\n\nYes. You cannot rely on calling read_object_file() in real-time when the\nfsck is being done by index-pack. The blob in question may be in the\npack you are indexing.\n\nSee fsck_finish() for how we do this for .gitmodules checks.\n\n-Peff\n"},{"id":"376375","messageId":"5f9ac691-4cb1-a15d-ca44-63fb64994d59@web.de","threadId":"51164","inReplyTo":"20190528190815.GA20499@sigill.intra.peff.net","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-05-28T23:34:32Z","receivedAt":"2019-05-28T23:34:51Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.05.19 um 21:08 schrieb Jeff King:\n> On Tue, May 28, 2019 at 08:01:43PM +0200, René Scharfe wrote:\n>\n>> Am 28.05.19 um 07:58 schrieb Jeff King:\n>>> On Sat, May 25, 2019 at 03:26:53PM +0200, René Scharfe wrote:\n>>>\n>>>> We could\n>>>> make git fsck report such symlinks.\n>>>\n>>> This is a little tricky, because fsck generally looks at individual\n>>> objects, and the bad pattern is a combination of a tree and a blob\n>>> together. I think you could make it work by reusing some of the code and\n>>> patterns from 9e84a6d758 (Merge branch 'jk/submodule-fsck-loose' into\n>>> maint, 2018-05-22).\n>>\n>> Actually it's super easy, barely an inconvenience (SCNR, watched a lot\n>> of those rants recently)..  Did I miss something?\n>\n> Yes. You cannot rely on calling read_object_file() in real-time when the\n> fsck is being done by index-pack. The blob in question may be in the\n> pack you are indexing.\n\nIt figures.\n\nSo something like the patch below?\n\nParsing trees with symlinks twice is not ideal, but keeps the set\nstructure simple -- a standard oidset suffices.\n\nThe global variables are ugly.  Moving them into struct fsck_option\nwould be possible, but not much better, as they aren't really\noptions.\n\nFSCK_MSG_MISSING_TREE_OBJECT has never been used before, it seems.\n\n---\n fsck.c          | 58 +++++++++++++++++++++++++++++++++++++++++++++++++\n t/t1450-fsck.sh | 13 +++++++++++\n 2 files changed, 71 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex 4703f55561..a6e7d0b03f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -19,6 +19,7 @@\n\n static struct oidset gitmodules_found = OIDSET_INIT;\n static struct oidset gitmodules_done = OIDSET_INIT;\n+static struct oidset trees_with_symlinks = OIDSET_INIT;\n\n #define FSCK_FATAL -1\n #define FSCK_INFO -2\n@@ -49,6 +50,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(MISSING_PARENT, ERROR) \\\n \tFUNC(MISSING_SPACE_BEFORE_DATE, ERROR) \\\n \tFUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \\\n+\tFUNC(MISSING_SYMLINK_OBJECT, ERROR) \\\n \tFUNC(MISSING_TAG, ERROR) \\\n \tFUNC(MISSING_TAG_ENTRY, ERROR) \\\n \tFUNC(MISSING_TAG_OBJECT, ERROR) \\\n@@ -58,6 +60,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(MISSING_TYPE_ENTRY, ERROR) \\\n \tFUNC(MULTIPLE_AUTHORS, ERROR) \\\n \tFUNC(TAG_OBJECT_NOT_TAG, ERROR) \\\n+\tFUNC(SYMLINK_OBJECT_NOT_BLOB, ERROR) \\\n \tFUNC(TREE_NOT_SORTED, ERROR) \\\n \tFUNC(UNKNOWN_TYPE, ERROR) \\\n \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n@@ -78,6 +81,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(NULL_SHA1, WARN) \\\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n+\tFUNC(NUL_IN_SYMLINK_TARGET, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n@@ -578,6 +582,33 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n\n+static int fsck_symlink(struct tree *tree, const char *name,\n+\t\t\tconst struct object_id *oid,\n+\t\t\tstruct fsck_options *options)\n+{\n+\tint ret = 0;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tvoid *buffer = read_object_file(oid, &type, &size);\n+\n+\tif (!buffer)\n+\t\tret = report(options, &tree->object,\n+\t\t\t     FSCK_MSG_MISSING_SYMLINK_OBJECT,\n+\t\t\t     \"cannot read blob object for symlink %s\", name);\n+\telse if (type != OBJ_BLOB)\n+\t\tret = report(options, &tree->object,\n+\t\t\t     FSCK_MSG_SYMLINK_OBJECT_NOT_BLOB,\n+\t\t\t     \"expected blob got %s for symlink %s\",\n+\t\t\t     type_name(type), name);\n+\telse if (memchr(buffer, '\\0', size))\n+\t\tret = report(options, &tree->object,\n+\t\t\t     FSCK_MSG_NUL_IN_SYMLINK_TARGET,\n+\t\t\t     \"NUL in target of symlink %s\", name);\n+\n+\tfree(buffer);\n+\treturn ret;\n+}\n+\n static int fsck_tree(struct tree *item, struct fsck_options *options)\n {\n \tint retval = 0;\n@@ -626,6 +657,8 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n+\t\tif (S_ISLNK(mode))\n+\t\t\toidset_insert(&trees_with_symlinks, &item->object.oid);\n\n \t\tif (update_tree_entry_gently(&desc)) {\n \t\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n@@ -1118,8 +1151,33 @@ int fsck_finish(struct fsck_options *options)\n \t\tfree(buf);\n \t}\n\n+\toidset_iter_init(&trees_with_symlinks, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct tree *tree;\n+\t\tstruct tree_desc desc;\n+\t\tstruct name_entry entry;\n+\n+\t\ttree = lookup_tree(the_repository, oid);\n+\t\tif (!tree) {\n+\t\t\tstruct object *obj = lookup_unknown_object(oid->hash);\n+\t\t\tret |= report(options, obj,\n+\t\t\t\t      FSCK_MSG_MISSING_TREE_OBJECT,\n+\t\t\t\t      \"tree %s not found\", oid_to_hex(oid));\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (parse_tree(tree))\n+\t\t\tcontinue;\n+\t\tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n+\t\t\tcontinue;\n+\t\twhile (tree_entry_gently(&desc, &entry)) {\n+\t\t\tif (S_ISLNK(entry.mode))\n+\t\t\t\tret |= fsck_symlink(tree, entry.path,\n+\t\t\t\t\t\t    &entry.oid, options);\n+\t\t}\n+\t}\n\n \toidset_clear(&gitmodules_found);\n \toidset_clear(&gitmodules_done);\n+\toidset_clear(&trees_with_symlinks);\n \treturn ret;\n }\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 0f268a3664..ce9501d063 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -822,4 +822,17 @@ test_expect_success 'detect corrupt index file in fsck' '\n \ttest_i18ngrep \"bad index file\" errors\n '\n\n+test_expect_success 'detect NUL in symlink target' '\n+\ttest_when_finished \"git update-ref -d refs/heads/nul_in_symlink\" &&\n+\ttest_when_finished \"remove_object \\$commit\" &&\n+\ttest_when_finished \"remove_object \\$tree\" &&\n+\ttest_when_finished \"remove_object \\$blob\" &&\n+\tblob=$(echo fooQbar | q_to_nul | git hash-object -w --stdin) &&\n+\ttree=$(echo \"120000 blob $blob\tsymlink\" | git mktree) &&\n+\tcommit=$(git commit-tree $tree) &&\n+\tgit update-ref refs/heads/nul_in_symlink $commit &&\n+\tgit fsck 2>out &&\n+\ttest_i18ngrep \"NUL in target of symlink\" out\n+'\n+\n test_done\n--\n2.21.0\n"},{"id":"376377","messageId":"20190529011744.GA22340@sigill.intra.peff.net","threadId":"51164","inReplyTo":"5f9ac691-4cb1-a15d-ca44-63fb64994d59@web.de","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-29T01:17:44Z","receivedAt":"2019-05-29T01:17:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 29, 2019 at 01:34:32AM +0200, René Scharfe wrote:\n\n> It figures.\n> \n> So something like the patch below?\n> \n> Parsing trees with symlinks twice is not ideal, but keeps the set\n> structure simple -- a standard oidset suffices.\n\nIf blobs comes after trees (and they usually do in a pack), you can do\nit in a single pass by marking the blob as a symlink target, and then\nwhen we actually see that blob's contents, marking it as either OK or\nproblematic. And then the finish() step just correlates those with the\ntree.\n\nIt does require O(n) storage in the number of symlinked blobs, but also\nO(n) in the number of symlinked tree entries (number of trees with\nsymlinks times the number of entries in each such tree, _even if they're\nthe same entry/blob as another tree).\n\nThat makes it a lot worse than the existing gitmodules check. There we\nonly care about finding the .gitmodules blobs. So even though you have a\nton of trees that mention .gitmodules (basically every root tree), the\nthe .gitmodules file itself doesn't change much. So we only end up with\na small oidset (and a small worst case for looking at objects twice).\n\nBut here the problem is in the tree, not the blob. So we're not finding\nsuspect blobs, but rather re-checking each tree. And no matter what we\ndo (whether it's visiting the object again, or creating a set or mapping\nwith the object names) is going to be linear there. And a repository\nwith a symlink in the root tree is going to revisit or put in our\nmapping every single root tree.\n\nTBH, I'm not sure this fsck check was worth it even without the\nimplementation complexity.\n\n> The global variables are ugly.  Moving them into struct fsck_option\n> would be possible, but not much better, as they aren't really\n> options.\n\nYeah, we use the name \"context\" elsewhere for this, which is a bit\nclearer. In a real object-oriented design, I guess people would make an\n\"Fscker\" object, and it would carry the options and context forward.\nThat's basically what our \"context\" objects are.\n\n> FSCK_MSG_MISSING_TREE_OBJECT has never been used before, it seems.\n\nYeah, this is leftover cruft from my gitmodules series. I double-checked\nthe early iterations to be sure, and it is definitely the case. I didn't\ndig down to find out the reason it went away, but I think it is probably\nthat the original version tried harder to find .gitmodules only in root\ntrees, and later we decided to just complain about it in any tree.\n\n-Peff\n"},{"id":"376408","messageId":"24574d74-b6ca-6d36-759f-32cf26b36c3f@web.de","threadId":"51164","inReplyTo":"20190529011744.GA22340@sigill.intra.peff.net","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-05-29T17:54:44Z","receivedAt":"2019-05-29T17:55:07Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.05.19 um 03:17 schrieb Jeff King:\n> On Wed, May 29, 2019 at 01:34:32AM +0200, René Scharfe wrote:\n>> Parsing trees with symlinks twice is not ideal, but keeps the set\n>> structure simple -- a standard oidset suffices.\n>\n> If blobs comes after trees (and they usually do in a pack), you can do\n> it in a single pass by marking the blob as a symlink target, and then\n> when we actually see that blob's contents, marking it as either OK or\n> problematic. And then the finish() step just correlates those with the\n> tree.\n\nGood idea.  Is that ordering guaranteed?  (Stumbling about the \"usually\"\nin your first sentence.)\n\nAn ordering where dependent objects (like trees) follow the objects they\nreference would be better suited for these kinds of checks..\n\n> It does require O(n) storage in the number of symlinked blobs, but also\n> O(n) in the number of symlinked tree entries (number of trees with\n> symlinks times the number of entries in each such tree, _even if they're\n> the same entry/blob as another tree).\n>\n> That makes it a lot worse than the existing gitmodules check. There we\n> only care about finding the .gitmodules blobs. So even though you have a\n> ton of trees that mention .gitmodules (basically every root tree), the\n> the .gitmodules file itself doesn't change much. So we only end up with\n> a small oidset (and a small worst case for looking at objects twice).\n>\n> But here the problem is in the tree, not the blob. So we're not finding\n> suspect blobs, but rather re-checking each tree. And no matter what we\n> do (whether it's visiting the object again, or creating a set or mapping\n> with the object names) is going to be linear there. And a repository\n> with a symlink in the root tree is going to revisit or put in our\n> mapping every single root tree.\n\nThat's true, potentially it needs remember and/or reprocess all trees,\nmeaning this check may double the run time of fsck in the worst case.\nExample from the wild: The kernel repo currently has 36 symlinks and\n6+ million objects are checked in total, and the symlink check processes\n18943 trees_with_symlinks entries there.\n\n> TBH, I'm not sure this fsck check was worth it even without the\n> implementation complexity.\n\nHmm.  git status reports such truncated symlinks as changed, so the\nissue *is* already detectable.\n\nRené\n"},{"id":"376436","messageId":"20190530115554.GB31607@sigill.intra.peff.net","threadId":"51164","inReplyTo":"24574d74-b6ca-6d36-759f-32cf26b36c3f@web.de","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-30T11:55:55Z","receivedAt":"2019-05-30T11:55:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 29, 2019 at 07:54:44PM +0200, René Scharfe wrote:\n\n> Am 29.05.19 um 03:17 schrieb Jeff King:\n> > On Wed, May 29, 2019 at 01:34:32AM +0200, René Scharfe wrote:\n> >> Parsing trees with symlinks twice is not ideal, but keeps the set\n> >> structure simple -- a standard oidset suffices.\n> >\n> > If blobs comes after trees (and they usually do in a pack), you can do\n> > it in a single pass by marking the blob as a symlink target, and then\n> > when we actually see that blob's contents, marking it as either OK or\n> > problematic. And then the finish() step just correlates those with the\n> > tree.\n> \n> Good idea.  Is that ordering guaranteed?  (Stumbling about the \"usually\"\n> in your first sentence.)\n\nIt's not guaranteed. Our implementation of pack-objects does order blobs\nafter trees, but I suspect this could be violated in rare cases with\nsome of the delta-island pack layering stuff.\n\nI think it makes sense to be sure that the receiver is correct no matter\nwhat, but optimize for this expected case (that's what I tried to do\nwith the .gitmodules checks).\n\n> An ordering where dependent objects (like trees) follow the objects they\n> reference would be better suited for these kinds of checks..\n\nBut worse for others (e.g., like .gitmodules where it's cheap to\nidentify a candidate blob, but the blob check is involved; there it's\nmuch more optimal to see the tree first).\n\n> > But here the problem is in the tree, not the blob. So we're not finding\n> > suspect blobs, but rather re-checking each tree. And no matter what we\n> > do (whether it's visiting the object again, or creating a set or mapping\n> > with the object names) is going to be linear there. And a repository\n> > with a symlink in the root tree is going to revisit or put in our\n> > mapping every single root tree.\n> \n> That's true, potentially it needs remember and/or reprocess all trees,\n> meaning this check may double the run time of fsck in the worst case.\n> Example from the wild: The kernel repo currently has 36 symlinks and\n> 6+ million objects are checked in total, and the symlink check processes\n> 18943 trees_with_symlinks entries there.\n\nThat sounds about right. It's basically every version of every tree that\nhas a symlink. Did it make a noticeable difference in timing? Indexing\nthe whole kernel history is already a horribly slow process. :)\n\n> > TBH, I'm not sure this fsck check was worth it even without the\n> > implementation complexity.\n> \n> Hmm.  git status reports such truncated symlinks as changed, so the\n> issue *is* already detectable.\n\nHmm, yeah. That makes sense, since the filesystem (well, really the\nsyscall API layer) cannot represent the data we are feeding it.\n\n-Peff\n"},{"id":"376552","messageId":"c961d89d-db0b-597f-c183-81aa791c0987@web.de","threadId":"51164","inReplyTo":"20190530115554.GB31607@sigill.intra.peff.net","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-06-02T16:58:48Z","receivedAt":"2019-06-02T16:59:38Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.05.19 um 13:55 schrieb Jeff King:\n> On Wed, May 29, 2019 at 07:54:44PM +0200, René Scharfe wrote:\n>\n>> Am 29.05.19 um 03:17 schrieb Jeff King:\n>>> But here the problem is in the tree, not the blob. So we're not finding\n>>> suspect blobs, but rather re-checking each tree. And no matter what we\n>>> do (whether it's visiting the object again, or creating a set or mapping\n>>> with the object names) is going to be linear there. And a repository\n>>> with a symlink in the root tree is going to revisit or put in our\n>>> mapping every single root tree.\n>>\n>> That's true, potentially it needs remember and/or reprocess all trees,\n>> meaning this check may double the run time of fsck in the worst case.\n>> Example from the wild: The kernel repo currently has 36 symlinks and\n>> 6+ million objects are checked in total, and the symlink check processes\n>> 18943 trees_with_symlinks entries there.\n>\n> That sounds about right. It's basically every version of every tree that\n> has a symlink. Did it make a noticeable difference in timing? Indexing\n> the whole kernel history is already a horribly slow process. :)\n\nRight, I didn't notice a difference -- no patience for watching that\nthing to the end.  But here are some numbers for v2.21.0 vs. master with\nthe patch:\n\nBenchmark #1: git fsck\n  Time (mean ± σ):     307.775 s ±  9.054 s    [User: 307.173 s, System: 0.448 s]\n  Range (min … max):   294.052 s … 322.931 s    10 runs\n\nBenchmark #2: ~/src/git/git fsck\n  Time (mean ± σ):     319.754 s ±  2.255 s    [User: 318.927 s, System: 0.671 s]\n  Range (min … max):   316.376 s … 323.747 s    10 runs\n\nSummary\n  'git fsck' ran\n    1.04 ± 0.03 times faster than '~/src/git/git fsck'\n\nSeeing only a single CPU core being stressed for that long is a bit sad\nto see.  Checking individual objects should be relatively easy to\nparallelize, shouldn't it?\n\nRené\n"},{"id":"376693","messageId":"20190604205342.GA27861@sigill.intra.peff.net","threadId":"51164","inReplyTo":"c961d89d-db0b-597f-c183-81aa791c0987@web.de","subject":"Re: git archive generates tar with malformed pax extended attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-04T20:53:42Z","receivedAt":"2019-06-04T20:53:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 02, 2019 at 06:58:48PM +0200, René Scharfe wrote:\n\n> > That sounds about right. It's basically every version of every tree that\n> > has a symlink. Did it make a noticeable difference in timing? Indexing\n> > the whole kernel history is already a horribly slow process. :)\n> \n> Right, I didn't notice a difference -- no patience for watching that\n> thing to the end.  But here are some numbers for v2.21.0 vs. master with\n> the patch:\n> \n> Benchmark #1: git fsck\n>   Time (mean ± σ):     307.775 s ±  9.054 s    [User: 307.173 s, System: 0.448 s]\n>   Range (min … max):   294.052 s … 322.931 s    10 runs\n> \n> Benchmark #2: ~/src/git/git fsck\n>   Time (mean ± σ):     319.754 s ±  2.255 s    [User: 318.927 s, System: 0.671 s]\n>   Range (min … max):   316.376 s … 323.747 s    10 runs\n> \n> Summary\n>   'git fsck' ran\n>     1.04 ± 0.03 times faster than '~/src/git/git fsck'\n\nI guess that's about what I'd expect. The bulk of the time in most repos\nwill go to fscking the actual blobs, I'd think. But hitting each tree\ntwice really is noticeable.\n\n> Seeing only a single CPU core being stressed for that long is a bit sad\n> to see.  Checking individual objects should be relatively easy to\n> parallelize, shouldn't it?\n\nYes. The fsck code is pretty old, and uses a very simple way of walking\nover all of the packs. index-pack (which backs verify-pack these days)\nis much smarter, and runs in parallel. It still takes a lock when doing\nthe actual fsck checks, but most of the time goes to the zlib inflation\nand delta reconstruction.\n\nThere's some discussion in:\n\n  https://public-inbox.org/git/20180816210657.GA9291@sigill.intra.peff.net/\n\nand even some patches elsewhere in the thread here:\n\n  https://public-inbox.org/git/20180902075528.GC18787@sigill.intra.peff.net/\n\nand here:\n\n  https://public-inbox.org/git/20180902085503.GA25391@sigill.intra.peff.net/\n\nI think the big show-stopper there is how ugly it is to run the pack\nverification in a separate process (and I suspect it is not just ugly\nfrom a code point of view, but actively breaks index-pack because it\nthen relies on the set of objects seen during the first phase to do its\nconnectivity check).\n\nSo there would probably need to be some lib-ification work on index-pack\nfirst, so that we could call it (at least in verification mode) multiple\ntimes from inside fsck.\n\n-Peff\n"}]}