patchAdd initial support for pax extended attributes

From: Pedro Alvarez Piedehierro <palvarez89@gmail.com>

Sometimes the tar files will contain pax extended attributes to deal with cases where the information needed doesn't fit in a standard ustar entry.

One of these cases is when the path is larger than 100 characters. A pax entry will appear containing two standard ustart entries. The first entry will have an 'x' typeflag, and contain the the extended attributes.

The pax extended attributes contain one or multiple records constructed as follows:

    "%d %s=%s\n", <length>, <keyword>, <value>

This commit makes sure that we always read the extended attibutes from pax entries, and in the case of finding one, we parse its records looking for 'path' information. If this information is found, it's stored to be used in the next ustar entry.

Information about the Pax Interchange Format can be found at:

    https://www.freebsd.org/cgi/man.cgi?manpath=FreeBSD+8-current&query=tar&sektion=5.

Before this change, importing gcc tarballs[1] would fail with the following error:

    fast-import crash report:
        fast-import process: 82899
        parent process     : 82897
        at 2018-05-21 12:35:27 +0000

    fatal: Unsupported command: 29 atime=1516870168.93527949

    Most Recent Commands Before Crash
    ---------------------------------
      M 644 :22495 gcc-7.3.0/libstdc++-v3/testsuite/20_util/duration/PaxHeaders.4467/comparison_operators
      M 644 :140367 gcc-7.3.0/gcc/ada/s-gloloc-mingw.adb
      M 644 :75143 gcc-7.3.0/gcc/testsuite/gcc.c-torture/execute/builtins/PaxHeaders.4467/strncat-chk-lib.c

      <snip>

      M 644 :135585 gcc-7.3.0/gcc/testsuite/c-c++-common/attr-warn-unused-result.c
      M 644 :54956 gcc-7.3.0/gcc/testsuite/go.test/test/fixedbugs/PaxHeaders.4467/bug335.dir
      M 644 :20632 27 mtime=1483272463.905435
    * 29 atime=1516870168.93527949

[1]: http://ftp.gnu.org/gnu/gcc/gcc-7.3.0/gcc-7.3.0.tar.xz

Signed-off-by: Pedro Alvarez <palvarez89@gmail.com>
---
 contrib/fast-import/import-tars.perl | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

Pedro Alvarez <pedro.alvarez@codethink.co.uk> writes:

Lead it with the name of the area you are adding support for pax ext header, e.g.

    Subject: [PATCH] import-tars: read overlong names from pax extended header

or something.

u-start? us-tart? sound yummy.  I think s/ustart entries/ustar headers/

s/contain/&s/

s/This commit makes sure/Make sure/;

Drop "Before this change, " and move the above to the very beginning of the proposed log message. The problem description is always "without this patch applied, we have this problem that needs to be fixed", so "Before this change" is an unnecessary thing to say.

The remainder of the crash log may or may not be in the problem description, if we want to shoot for brevity. If I were writing a log message for this patch, I'd go for even shorter version, e.g.

	Importing gcc tarballs[1] with import-tars script (in
	contrib/) fails when hitting a pax extended header that
	records a long pathname.

	Teach the code to parse and grab information from pax
	extended headers, and reconstruct a long pathname that is
	split into multiple records, to correct this problem.

        The code to parse pax extended headers were written,
        consulting the Pax Interchange Format documentation [2].

	[1] http://ftp.gnu.org/gnu/gcc/gcc-7.3.0/gcc-7.3.0.tar.xz
	[2] https://www.freebsd.org/cgi/man.cgi?manpath=FreeBSD+8-current&query=tar&sektion=5

Would we ever get a short-read (i.e. we ask to read 512 bytes, syscall returns after reading only 256 bytes, even though next call to read would give the remaining 256 bytes and later ones)?

If we do get a short-read, would that be an error (in which case, how are we handling it)? If it is not an error, should we continue reading, instead of leaving the loop?

It is a very good idea to add comment on why we are ignoring and doing 'next' here, but the tar format documentation you cited never uses the word 'folder'. s/folder/directory/, probably.

Thanks.

On Wed, May 23, 2018 at 11:34:52AM +0900, Junio C Hamano wrote:

No, because perl's read() is buffered (you need sysread() to get a real syscall read). We might read fewer than 512 if we hit EOF, but I think that would be a truncated input, then, since ustar does everything in 512-byte records.

I do think we'd fail to notice the truncation, which isn't ideal. But it looks like the rest of the script suffers from the same issue.

If anybody cares, it might not be too hard to wrap all of the 512-byte read calls into a helper that dies on bogus input. I sort of assumed this was mostly a proof of concept script and nobody used it, though. :)

It makes me wonder if there is a better-tested tar-reading module in CPAN that could be used (though at the expense of requiring an extra dependency).

-Peff

Jeff King <peff@peff.net> writes:

Perhaps. In any case, the patch presented here is an improvement over the status quo, so let's move the world forward by taking it without any further "while at it" fixes, which can come later when people feel inclined to do so.

Thanks, both, for writing and reviewing ;-)

From: Pedro Alvarez Piedehierro <palvarez89@gmail.com>

Hello!

In this version I've trimmed and improved the commit message as suggested.

Regarding the error handling, as Jeff mentioned, could be improved in general in the entire script. But I guess I could do it if needed to get this patch approved.

Thanks for reviewing and giving me some feedback!

Pedro.

Pedro Alvarez Piedehierro (1):
  import-tars: read overlong names from pax extended header

 contrib/fast-import/import-tars.perl | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

From: Pedro Alvarez Piedehierro <palvarez89@gmail.com>

Importing gcc tarballs[1] with import-tars script (in contrib) fails when hitting a pax extended header.

Make sure we always read the extended attributes from the pax entries, and store the 'path' value if found to be used in the next ustar entry.

The code to parse pax extended headers was written consulting the Pax Pax Interchange Format documentation [2].

[1] http://ftp.gnu.org/gnu/gcc/gcc-7.3.0/gcc-7.3.0.tar.xz [2] https://www.freebsd.org/cgi/man.cgi?manpath=FreeBSD+8-current&query=tar&sektion=5

Signed-off-by: Pedro Alvarez <palvarez89@gmail.com>
---
 contrib/fast-import/import-tars.perl | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)