{"thread":{"id":"48545","subject":"[PATCH] Add initial support for pax extended attributes","startedAt":"2018-05-22T10:06:18Z","lastAt":"2018-05-23T23:38:56Z","messageCount":6,"participants":["Pedro Alvarez","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"348289","messageId":"20180522100548.29881-1-pedro.alvarez@codethink.co.uk","threadId":"48545","inReplyTo":null,"subject":"[PATCH] Add initial support for pax extended attributes","fromName":"Pedro Alvarez","fromEmail":"pedro.alvarez@codethink.co.uk","sentAt":"2018-05-22T10:05:48Z","receivedAt":"2018-05-22T10:06:18Z","isPatch":true,"sender":{"key":"pedro.alvarez@codethink.co.uk","avatar":null},"body":"From: Pedro Alvarez Piedehierro <palvarez89@gmail.com>\n\nSometimes the tar files will contain pax extended attributes to deal\nwith cases where the information needed doesn't fit in a standard\nustar entry.\n\nOne of these cases is when the path is larger than 100 characters. A\npax entry will appear containing two standard ustart entries. The first\nentry will have an 'x' typeflag, and contain the the extended attributes.\n\nThe pax extended attributes contain one or multiple records constructed as\nfollows:\n\n    \"%d %s=%s\\n\", <length>, <keyword>, <value>\n\nThis commit makes sure that we always read the extended attibutes from\npax entries, and in the case of finding one, we parse its records\nlooking for 'path' information. If this information is found, it's\nstored to be used in the next ustar entry.\n\nInformation about the Pax Interchange Format can be found at:\n\n    https://www.freebsd.org/cgi/man.cgi?manpath=FreeBSD+8-current&query=tar&sektion=5.\n\nBefore this change, importing gcc tarballs[1] would fail with the\nfollowing error:\n\n    fast-import crash report:\n        fast-import process: 82899\n        parent process     : 82897\n        at 2018-05-21 12:35:27 +0000\n\n    fatal: Unsupported command: 29 atime=1516870168.93527949\n\n    Most Recent Commands Before Crash\n    ---------------------------------\n      M 644 :22495 gcc-7.3.0/libstdc++-v3/testsuite/20_util/duration/PaxHeaders.4467/comparison_operators\n      M 644 :140367 gcc-7.3.0/gcc/ada/s-gloloc-mingw.adb\n      M 644 :75143 gcc-7.3.0/gcc/testsuite/gcc.c-torture/execute/builtins/PaxHeaders.4467/strncat-chk-lib.c\n\n      <snip>\n\n      M 644 :135585 gcc-7.3.0/gcc/testsuite/c-c++-common/attr-warn-unused-result.c\n      M 644 :54956 gcc-7.3.0/gcc/testsuite/go.test/test/fixedbugs/PaxHeaders.4467/bug335.dir\n      M 644 :20632 27 mtime=1483272463.905435\n    * 29 atime=1516870168.93527949\n\n[1]: http://ftp.gnu.org/gnu/gcc/gcc-7.3.0/gcc-7.3.0.tar.xz\n\nSigned-off-by: Pedro Alvarez <palvarez89@gmail.com>\n---\n contrib/fast-import/import-tars.perl | 32 ++++++++++++++++++++++++++++++--\n 1 file changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/fast-import/import-tars.perl b/contrib/fast-import/import-tars.perl\nindex d60b4315ed..c2e54ec7a3 100755\n--- a/contrib/fast-import/import-tars.perl\n+++ b/contrib/fast-import/import-tars.perl\n@@ -63,6 +63,8 @@ foreach my $tar_file (@ARGV)\n \tmy $have_top_dir = 1;\n \tmy ($top_dir, %files);\n \n+\tmy $next_path = '';\n+\n \twhile (read(I, $_, 512) == 512) {\n \t\tmy ($name, $mode, $uid, $gid, $size, $mtime,\n \t\t\t$chksum, $typeflag, $linkname, $magic,\n@@ -70,6 +72,13 @@ foreach my $tar_file (@ARGV)\n \t\t\t$prefix) = unpack 'Z100 Z8 Z8 Z8 Z12 Z12\n \t\t\tZ8 Z1 Z100 Z6\n \t\t\tZ2 Z32 Z32 Z8 Z8 Z*', $_;\n+\n+\t\tunless ($next_path eq '') {\n+\t\t\t# Recover name from previous extended header\n+\t\t\t$name = $next_path;\n+\t\t\t$next_path = '';\n+\t\t}\n+\n \t\tlast unless length($name);\n \t\tif ($name eq '././@LongLink') {\n \t\t\t# GNU tar extension\n@@ -90,13 +99,32 @@ foreach my $tar_file (@ARGV)\n \t\t\tZ8 Z1 Z100 Z6\n \t\t\tZ2 Z32 Z32 Z8 Z8 Z*', $_;\n \t\t}\n-\t\tnext if $name =~ m{/\\z};\n \t\t$mode = oct $mode;\n \t\t$size = oct $size;\n \t\t$mtime = oct $mtime;\n \t\tnext if $typeflag == 5; # directory\n \n-\t\tif ($typeflag != 1) { # handle hard links later\n+\t\tif ($typeflag eq 'x') { # extended header\n+\t\t\t# If extended header, check for path\n+\t\t\tmy $pax_header = '';\n+\t\t\twhile ($size > 0 && read(I, $_, 512) == 512) {\n+\t\t\t\t$pax_header = $pax_header . substr($_, 0, $size);\n+\t\t\t\t$size -= 512;\n+\t\t\t}\n+\n+\t\t\tmy @lines = split /\\n/, $pax_header;\n+\t\t\tforeach my $line (@lines) {\n+\t\t\t\tmy ($len, $entry) = split / /, $line;\n+\t\t\t\tmy ($key, $value) = split /=/, $entry;\n+\t\t\t\tif ($key eq 'path') {\n+\t\t\t\t\t$next_path = $value;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tnext;\n+\t\t} elsif ($name =~ m{/\\z}) {\n+\t\t\t# If it's a folder, ignore\n+\t\t\tnext;\n+\t\t} elsif ($typeflag != 1) { # handle hard links later\n \t\t\tprint FI \"blob\\n\", \"mark :$next_mark\\n\";\n \t\t\tif ($typeflag == 2) { # symbolic link\n \t\t\t\tprint FI \"data \", length($linkname), \"\\n\",\n-- \n2.11.0\n\n"},{"id":"348346","messageId":"xmqqd0xnw14j.fsf@gitster-ct.c.googlers.com","threadId":"48545","inReplyTo":"20180522100548.29881-1-pedro.alvarez@codethink.co.uk","subject":"Re: [PATCH] Add initial support for pax extended attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-23T02:34:52Z","receivedAt":"2018-05-23T02:35:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pedro Alvarez <pedro.alvarez@codethink.co.uk> writes:\n\n> From: Pedro Alvarez Piedehierro <palvarez89@gmail.com>\n> Subject: [PATCH] Add initial support for pax extended attributes\n\nLead it with the name of the area you are adding support for pax ext\nheader, e.g.\n\n    Subject: [PATCH] import-tars: read overlong names from pax extended header\n\nor something.\n\n> Sometimes the tar files will contain pax extended attributes to deal\n> with cases where the information needed doesn't fit in a standard\n> ustar entry.\n>\n> One of these cases is when the path is larger than 100 characters. A\n> pax entry will appear containing two standard ustart entries. The first\n\nu-start? us-tart? sound yummy.  I think s/ustart entries/ustar headers/\n\n> entry will have an 'x' typeflag, and contain the the extended attributes.\n\ns/contain/&s/\n\n>\n> The pax extended attributes contain one or multiple records constructed as\n> follows:\n>\n>     \"%d %s=%s\\n\", <length>, <keyword>, <value>\n\n> This commit makes sure that we always read the extended attibutes from\n\ns/This commit makes sure/Make sure/;\n\n> pax entries, and in the case of finding one, we parse its records\n> looking for 'path' information. If this information is found, it's\n> stored to be used in the next ustar entry.\n>\n> Information about the Pax Interchange Format can be found at:\n>\n>     https://www.freebsd.org/cgi/man.cgi?manpath=FreeBSD+8-current&query=tar&sektion=5.\n\n> Before this change, importing gcc tarballs[1] would fail with the\n> following error:\n>\n>     fast-import crash report:\n>         fast-import process: 82899\n>         parent process     : 82897\n>         at 2018-05-21 12:35:27 +0000\n>\n>     fatal: Unsupported command: 29 atime=1516870168.93527949\n\nDrop \"Before this change, \" and move the above to the very beginning\nof the proposed log message.  The problem description is always\n\"without this patch applied, we have this problem that needs to be\nfixed\", so \"Before this change\" is an unnecessary thing to say.\n\nThe remainder of the crash log may or may not be in the problem\ndescription, if we want to shoot for brevity.  If I were writing a\nlog message for this patch, I'd go for even shorter version, e.g.\n\n\tImporting gcc tarballs[1] with import-tars script (in\n\tcontrib/) fails when hitting a pax extended header that\n\trecords a long pathname.\n\n\tTeach the code to parse and grab information from pax\n\textended headers, and reconstruct a long pathname that is\n\tsplit into multiple records, to correct this problem.\n\n        The code to parse pax extended headers were written,\n        consulting the Pax Interchange Format documentation [2].\n\n\t[1] http://ftp.gnu.org/gnu/gcc/gcc-7.3.0/gcc-7.3.0.tar.xz\n\t[2] https://www.freebsd.org/cgi/man.cgi?manpath=FreeBSD+8-current&query=tar&sektion=5\n\n> index d60b4315ed..c2e54ec7a3 100755\n> --- a/contrib/fast-import/import-tars.perl\n> +++ b/contrib/fast-import/import-tars.perl\n> @@ -63,6 +63,8 @@ foreach my $tar_file (@ARGV)\n>  \tmy $have_top_dir = 1;\n>  \tmy ($top_dir, %files);\n>  \n> +\tmy $next_path = '';\n> +\n>  \twhile (read(I, $_, 512) == 512) {\n>  \t\tmy ($name, $mode, $uid, $gid, $size, $mtime,\n>  \t\t\t$chksum, $typeflag, $linkname, $magic,\n> @@ -70,6 +72,13 @@ foreach my $tar_file (@ARGV)\n>  \t\t\t$prefix) = unpack 'Z100 Z8 Z8 Z8 Z12 Z12\n>  \t\t\tZ8 Z1 Z100 Z6\n>  \t\t\tZ2 Z32 Z32 Z8 Z8 Z*', $_;\n> +\n> +\t\tunless ($next_path eq '') {\n> +\t\t\t# Recover name from previous extended header\n> +\t\t\t$name = $next_path;\n> +\t\t\t$next_path = '';\n> +\t\t}\n> +\n>  \t\tlast unless length($name);\n>  \t\tif ($name eq '././@LongLink') {\n>  \t\t\t# GNU tar extension\n> @@ -90,13 +99,32 @@ foreach my $tar_file (@ARGV)\n>  \t\t\tZ8 Z1 Z100 Z6\n>  \t\t\tZ2 Z32 Z32 Z8 Z8 Z*', $_;\n>  \t\t}\n> -\t\tnext if $name =~ m{/\\z};\n>  \t\t$mode = oct $mode;\n>  \t\t$size = oct $size;\n>  \t\t$mtime = oct $mtime;\n>  \t\tnext if $typeflag == 5; # directory\n>  \n> -\t\tif ($typeflag != 1) { # handle hard links later\n> +\t\tif ($typeflag eq 'x') { # extended header\n> +\t\t\t# If extended header, check for path\n> +\t\t\tmy $pax_header = '';\n> +\t\t\twhile ($size > 0 && read(I, $_, 512) == 512) {\n\nWould we ever get a short-read (i.e. we ask to read 512 bytes,\nsyscall returns after reading only 256 bytes, even though next call\nto read would give the remaining 256 bytes and later ones)?\n\nIf we do get a short-read, would that be an error (in which case,\nhow are we handling it)?  If it is not an error, should we continue\nreading, instead of leaving the loop?\n\n> +\t\t\t\t$pax_header = $pax_header . substr($_, 0, $size);\n> +\t\t\t\t$size -= 512;\n> +\t\t\t}\n> +\n> +\t\t\tmy @lines = split /\\n/, $pax_header;\n> +\t\t\tforeach my $line (@lines) {\n> +\t\t\t\tmy ($len, $entry) = split / /, $line;\n> +\t\t\t\tmy ($key, $value) = split /=/, $entry;\n> +\t\t\t\tif ($key eq 'path') {\n> +\t\t\t\t\t$next_path = $value;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t\tnext;\n> +\t\t} elsif ($name =~ m{/\\z}) {\n> +\t\t\t# If it's a folder, ignore\n\nIt is a very good idea to add comment on why we are ignoring and\ndoing 'next' here, but the tar format documentation you cited never\nuses the word 'folder'.  s/folder/directory/, probably.\n\n> +\t\t\tnext;\n> +\t\t} elsif ($typeflag != 1) { # handle hard links later\n>  \t\t\tprint FI \"blob\\n\", \"mark :$next_mark\\n\";\n>  \t\t\tif ($typeflag == 2) { # symbolic link\n>  \t\t\t\tprint FI \"data \", length($linkname), \"\\n\",\n\nThanks.\n"},{"id":"348351","messageId":"20180523045714.GA5567@sigill.intra.peff.net","threadId":"48545","inReplyTo":"xmqqd0xnw14j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Add initial support for pax extended attributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-23T04:57:15Z","receivedAt":"2018-05-23T04:57:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 23, 2018 at 11:34:52AM +0900, Junio C Hamano wrote:\n\n> > @@ -90,13 +99,32 @@ foreach my $tar_file (@ARGV)\n> >  \t\t\tZ8 Z1 Z100 Z6\n> >  \t\t\tZ2 Z32 Z32 Z8 Z8 Z*', $_;\n> >  \t\t}\n> > -\t\tnext if $name =~ m{/\\z};\n> >  \t\t$mode = oct $mode;\n> >  \t\t$size = oct $size;\n> >  \t\t$mtime = oct $mtime;\n> >  \t\tnext if $typeflag == 5; # directory\n> >  \n> > -\t\tif ($typeflag != 1) { # handle hard links later\n> > +\t\tif ($typeflag eq 'x') { # extended header\n> > +\t\t\t# If extended header, check for path\n> > +\t\t\tmy $pax_header = '';\n> > +\t\t\twhile ($size > 0 && read(I, $_, 512) == 512) {\n> \n> Would we ever get a short-read (i.e. we ask to read 512 bytes,\n> syscall returns after reading only 256 bytes, even though next call\n> to read would give the remaining 256 bytes and later ones)?\n\nNo, because perl's read() is buffered (you need sysread() to get a real\nsyscall read). We might read fewer than 512 if we hit EOF, but I think\nthat would be a truncated input, then, since ustar does everything in\n512-byte records.\n\nI do think we'd fail to notice the truncation, which isn't ideal. But it\nlooks like the rest of the script suffers from the same issue.\n\nIf anybody cares, it might not be too hard to wrap all of the 512-byte\nread calls into a helper that dies on bogus input. I sort of assumed\nthis was mostly a proof of concept script and nobody used it, though. :)\n\nIt makes me wonder if there is a better-tested tar-reading module in\nCPAN that could be used (though at the expense of requiring an extra\ndependency).\n\n-Peff\n"},{"id":"348396","messageId":"20180523225417.10165-1-pedro.alvarez@codethink.co.uk","threadId":"48545","inReplyTo":"20180522100548.29881-1-pedro.alvarez@codethink.co.uk","subject":"[PATCH v2 0/1] import-tars: read overlong names from pax extended header","fromName":"Pedro Alvarez","fromEmail":"pedro.alvarez@codethink.co.uk","sentAt":"2018-05-23T22:54:16Z","receivedAt":"2018-05-23T22:54:34Z","isPatch":true,"sender":{"key":"pedro.alvarez@codethink.co.uk","avatar":null},"body":"From: Pedro Alvarez Piedehierro <palvarez89@gmail.com>\n\nHello!\n\nIn this version I've trimmed and improved the commit message as suggested.\n\nRegarding the error handling, as Jeff mentioned, could be improved\nin general in the entire script. But I guess I could do it if needed\nto get this patch approved.\n\nThanks for reviewing and giving me some feedback!\n\nPedro.\n\nPedro Alvarez Piedehierro (1):\n  import-tars: read overlong names from pax extended header\n\n contrib/fast-import/import-tars.perl | 31 +++++++++++++++++++++++++++++--\n 1 file changed, 29 insertions(+), 2 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"348397","messageId":"20180523225417.10165-2-pedro.alvarez@codethink.co.uk","threadId":"48545","inReplyTo":"20180523225417.10165-1-pedro.alvarez@codethink.co.uk","subject":"[PATCH v2 1/1] import-tars: read overlong names from pax extended header","fromName":"Pedro Alvarez","fromEmail":"pedro.alvarez@codethink.co.uk","sentAt":"2018-05-23T22:54:17Z","receivedAt":"2018-05-23T22:54:37Z","isPatch":true,"sender":{"key":"pedro.alvarez@codethink.co.uk","avatar":null},"body":"From: Pedro Alvarez Piedehierro <palvarez89@gmail.com>\n\nImporting gcc tarballs[1] with import-tars script (in contrib) fails\nwhen hitting a pax extended header.\n\nMake sure we always read the extended attributes from the pax entries,\nand store the 'path' value if found to be used in the next ustar entry.\n\nThe code to parse pax extended headers was written consulting the Pax\nPax Interchange Format documentation [2].\n\n[1] http://ftp.gnu.org/gnu/gcc/gcc-7.3.0/gcc-7.3.0.tar.xz\n[2] https://www.freebsd.org/cgi/man.cgi?manpath=FreeBSD+8-current&query=tar&sektion=5\n\nSigned-off-by: Pedro Alvarez <palvarez89@gmail.com>\n---\n contrib/fast-import/import-tars.perl | 31 +++++++++++++++++++++++++++++--\n 1 file changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/fast-import/import-tars.perl b/contrib/fast-import/import-tars.perl\nindex d60b4315ed..e800d9f5c9 100755\n--- a/contrib/fast-import/import-tars.perl\n+++ b/contrib/fast-import/import-tars.perl\n@@ -63,6 +63,8 @@ foreach my $tar_file (@ARGV)\n \tmy $have_top_dir = 1;\n \tmy ($top_dir, %files);\n \n+\tmy $next_path = '';\n+\n \twhile (read(I, $_, 512) == 512) {\n \t\tmy ($name, $mode, $uid, $gid, $size, $mtime,\n \t\t\t$chksum, $typeflag, $linkname, $magic,\n@@ -70,6 +72,13 @@ foreach my $tar_file (@ARGV)\n \t\t\t$prefix) = unpack 'Z100 Z8 Z8 Z8 Z12 Z12\n \t\t\tZ8 Z1 Z100 Z6\n \t\t\tZ2 Z32 Z32 Z8 Z8 Z*', $_;\n+\n+\t\tunless ($next_path eq '') {\n+\t\t\t# Recover name from previous extended header\n+\t\t\t$name = $next_path;\n+\t\t\t$next_path = '';\n+\t\t}\n+\n \t\tlast unless length($name);\n \t\tif ($name eq '././@LongLink') {\n \t\t\t# GNU tar extension\n@@ -90,13 +99,31 @@ foreach my $tar_file (@ARGV)\n \t\t\tZ8 Z1 Z100 Z6\n \t\t\tZ2 Z32 Z32 Z8 Z8 Z*', $_;\n \t\t}\n-\t\tnext if $name =~ m{/\\z};\n \t\t$mode = oct $mode;\n \t\t$size = oct $size;\n \t\t$mtime = oct $mtime;\n \t\tnext if $typeflag == 5; # directory\n \n-\t\tif ($typeflag != 1) { # handle hard links later\n+\t\tif ($typeflag eq 'x') { # extended header\n+\t\t\t# If extended header, check for path\n+\t\t\tmy $pax_header = '';\n+\t\t\twhile ($size > 0 && read(I, $_, 512) == 512) {\n+\t\t\t\t$pax_header = $pax_header . substr($_, 0, $size);\n+\t\t\t\t$size -= 512;\n+\t\t\t}\n+\n+\t\t\tmy @lines = split /\\n/, $pax_header;\n+\t\t\tforeach my $line (@lines) {\n+\t\t\t\tmy ($len, $entry) = split / /, $line;\n+\t\t\t\tmy ($key, $value) = split /=/, $entry;\n+\t\t\t\tif ($key eq 'path') {\n+\t\t\t\t\t$next_path = $value;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tnext;\n+\t\t} elsif ($name =~ m{/\\z}) { # directory\n+\t\t\tnext;\n+\t\t} elsif ($typeflag != 1) { # handle hard links later\n \t\t\tprint FI \"blob\\n\", \"mark :$next_mark\\n\";\n \t\t\tif ($typeflag == 2) { # symbolic link\n \t\t\t\tprint FI \"data \", length($linkname), \"\\n\",\n-- \n2.11.0\n\n"},{"id":"348399","messageId":"xmqq1se2uelx.fsf@gitster-ct.c.googlers.com","threadId":"48545","inReplyTo":"20180523045714.GA5567@sigill.intra.peff.net","subject":"Re: [PATCH] Add initial support for pax extended attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-23T23:38:50Z","receivedAt":"2018-05-23T23:38:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I do think we'd fail to notice the truncation, which isn't ideal. But it\n> looks like the rest of the script suffers from the same issue.\n>\n> If anybody cares, it might not be too hard to wrap all of the 512-byte\n> read calls into a helper that dies on bogus input.\n\nPerhaps.  In any case, the patch presented here is an improvement\nover the status quo, so let's move the world forward by taking it\nwithout any further \"while at it\" fixes, which can come later when\npeople feel inclined to do so.\n\nThanks, both, for writing and reviewing ;-)\n"}]}