git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB

From
René Scharfe <l.s.r@web.de>
Date
Jun 20, 2016, 22:54 UTC
Message-ID
<57687413.3030609@web.de>
In-Reply-To
<20160616043733.GA18323@sigill.intra.peff.net>
Am 16.06.2016 um 06:37 schrieb Jeff King:
Show 77 quoted lines
> The ustar format has a fixed-length field for the size of
> each file entry which is supposed to contain up to 11 bytes
> of octal-formatted data plus a NUL or space terminator.
>
> These means that the largest size we can represent is
> 077777777777, or 1 byte short of 8GB. The correct solution
> for a larger file, according to POSIX.1-2001, is to add an
> extended pax header, similar to how we handle long
> filenames. This patch does that, and writes zero for the
> size field in the ustar header (the last bit is not
> mentioned by POSIX, but it matches how GNU tar behaves with
> --format=pax).
>
> This should be a strict improvement over the current
> behavior, which is to die in xsnprintf with a "BUG".
> However, there's some interesting history here.
>
> Prior to f2f0267 (archive-tar: use xsnprintf for trivial
> formatting, 2015-09-24), we silently overflowed the "size"
> field. The extra bytes ended up in the "mtime" field of the
> header, which was then immediately written itself,
> overwriting our extra bytes. What that means depends on how
> many bytes we wrote.
>
> If the size was 64GB or greater, then we actually overflowed
> digits into the mtime field, meaning our value was was
> effectively right-shifted by those lost octal digits. And
> this patch is again a strict improvement over that.
>
> But if the size was between 8GB and 64GB, then our 12-byte
> field held all of the actual digits, and only our NUL
> terminator overflowed. According to POSIX, there should be a
> NUL or space at the end of the field. However, GNU tar seems
> to be lenient here, and will correctly parse a size up 64GB
> (minus one) from the field. So sizes in this range might
> have just worked, depending on the implementation reading
> the tarfile.
>
> This patch is mostly still an improvement there, as the 8GB
> limit is specifically mentioned in POSIX as the correct
> limit. But it's possible that it could be a regression
> (versus the pre-f2f0267 state) if all of the following are
> true:
>
>    1. You have a file between 8GB and 64GB.
>
>    2. Your tar implementation _doesn't_ know about pax
>       extended headers.
>
>    3. Your tar implementation _does_ parse 12-byte sizes from
>       the ustar header without a delimiter.
>
> It's probably not worth worrying about such an obscure set
> of conditions, but I'm documenting it here just in case.
>
> There's no test included here. I did confirm that this works
> (using GNU tar) with:
>
>    $ dd if=/dev/zero seek=64G bs=1 count=1 of=huge
>    $ git add huge
>    $ git commit -q -m foo
>    $ git archive HEAD | head -c 10000 | tar tvf - 2>/dev/null
>    -rw-rw-r-- root/root 68719476737 2016-06-15 21:07 huge
>
> Pre-f2f0267, this would yield a bogus size of 8GB, and
> post-f2f0267, git-archive simply dies.
>
> Unfortunately, it's quite an expensive test to run. For one
> thing, unless your filesystem supports files with holes, it
> takes 64GB of disk space (you might think piping straight to
> `hash-object --stdin` would be better, but it's not; that
> tries to buffer all 64GB in RAM!). Furthermore, hashing and
> compressing the object takes several minutes of CPU time.
>
> We could ship just the resulting compressed object data as a
> loose object, but even that takes 64MB. So sadly, this code
> path remains untested in the test suite.

If we could set the limit to a lower value than 8GB for testing then we could at least check if the extended header is written, e.g. if ustar_size() could be convinced to return 0 every time using a hidden command line parameter or an environment variable or something better.

Show 38 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   archive-tar.c | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index cb99df2..7340b64 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,
>   	strbuf_addch(sb, '\n');
>   }
>
> +/*
> + * Like strbuf_append_ext_header, but for numeric values.
> + */
> +static void strbuf_append_ext_header_uint(struct strbuf *sb,
> +					  const char *keyword,
> +					  uintmax_t value)
> +{
> +	char buf[40]; /* big enough for 2^128 in decimal, plus NUL */
> +	int len;
> +
> +	len = xsnprintf(buf, sizeof(buf), "%"PRIuMAX, value);
> +	strbuf_append_ext_header(sb, keyword, buf, len);
> +}
> +
>   static unsigned int ustar_header_chksum(const struct ustar_header *header)
>   {
>   	const unsigned char *p = (const unsigned char *)header;
> @@ -163,12 +177,21 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)
>   	return i;
>   }
>
> +static inline unsigned long ustar_size(uintmax_t size)
> +{
> +	if (size < 077777777777UL)
Shouldn't that be less-or-equal?
Show 22 quoted lines
> +		return size;
> +	else
> +		return 0;
> +}
> +
>   static void prepare_header(struct archiver_args *args,
>   			   struct ustar_header *header,
>   			   unsigned int mode, unsigned long size)
>   {
>   	xsnprintf(header->mode, sizeof(header->mode), "%07o", mode & 07777);
> -	xsnprintf(header->size, sizeof(header->size), "%011lo", S_ISREG(mode) ? size : 0);
> +	xsnprintf(header->size, sizeof(header->size), "%011lo",
> +		  S_ISREG(mode) ? ustar_size(size) : 0);
>   	xsnprintf(header->mtime, sizeof(header->mtime), "%011lo", (unsigned long) args->time);
>
>   	xsnprintf(header->uid, sizeof(header->uid), "%07o", 0);
> @@ -267,6 +290,9 @@ static int write_tar_entry(struct archiver_args *args,
>   			memcpy(header.linkname, buffer, size);
>   	}
>
> +	if (ustar_size(size) != size)
> +		strbuf_append_ext_header_uint(&ext_header, "size", size);

It needs "S_ISREG(mode) && " as well, no? In practice it probably doesn't matter (until someone stores a 8GB long symlink target), but the size field should only be set for regular files.

Show 5 quoted lines
> +
>   	prepare_header(args, &header, mode, size);
>
>   	if (ext_header.len > 0) {
>
Previous: Jeff KingNext: Jeff King
Message 3 of 61 in “friendlier handling of overflows in archive-tar”
  1. 0/2 friendlier handling of overflows in archive-tarJeff King, Jun 16, 2016
  2. 1/2 archive-tar: write extended headers for file sizes >= 8GBJeff King, Jun 16, 2016
  3. René ScharfeJun 20, 2016
  4. Jeff KingJun 21, 2016
  5. Jeff KingJun 21, 2016
  6. René ScharfeJun 21, 2016
  7. René ScharfeJun 21, 2016
  8. Jeff KingJun 21, 2016
  9. René ScharfeJun 22, 2016
  10. Jeff KingJun 21, 2016
  11. René ScharfeJun 22, 2016
  12. Jeff KingJun 23, 2016
  13. René ScharfeJun 21, 2016
  14. Robin H. JohnsonJun 21, 2016
  15. Jeff KingJun 21, 2016
  16. 2/2 archive-tar: write extended headers for far-future mtimeJeff King, Jun 16, 2016
  17. René ScharfeJun 20, 2016
  18. René ScharfeJun 22, 2016
  19. Jeff KingJun 23, 2016
  20. René ScharfeJun 23, 2016
  21. Jeff KingJun 23, 2016
  22. Junio C HamanoJun 16, 2016
  23. Jeff KingJun 21, 2016
  24. 1/2 archive-tar: write extended headers for file sizes >= 8GBJeff King, Jun 21, 2016
  25. 2/2 archive-tar: write extended headers for far-future mtimeJeff King, Jun 21, 2016
  26. Junio C HamanoJun 21, 2016
  27. friendlier handling of overflows in archive-tarJeff King, Jun 23, 2016
  28. 1/4 t5000: test tar files that overflow ustar headersJeff King, Jun 23, 2016
  29. Jeff KingJun 23, 2016
  30. Johannes SixtJun 24, 2016
  31. Jeff KingJun 24, 2016
  32. Johannes SixtJun 24, 2016
  33. 0/4 portable signal-checking in testsJeff King, Jun 24, 2016
  34. 1/4 tests: factor portable signal check out of t0005Jeff King, Jun 24, 2016
  35. Johannes SixtJun 24, 2016
  36. Jeff KingJun 24, 2016
  37. Johannes SixtJun 24, 2016
  38. 2/4 t0005: use test_match_signal as appropriateJeff King, Jun 24, 2016
  39. 3/4 test_must_fail: use test_match_signalJeff King, Jun 24, 2016
  40. 4/4 t/lib-git-daemon: use test_match_signalJeff King, Jun 24, 2016
  41. Jeff KingJun 24, 2016
  42. Junio C HamanoJun 24, 2016
  43. Jeff KingJun 24, 2016
  44. Junio C HamanoJun 24, 2016
  45. Eric SunshineJun 24, 2016
  46. Jeff KingJun 24, 2016
  47. Jeff KingJun 24, 2016
  48. Junio C HamanoJun 24, 2016
  49. Jeff KingJun 24, 2016
  50. 2/4 archive-tar: write extended headers for file sizes >= 8GBJeff King, Jun 23, 2016
  51. Junio C HamanoJun 24, 2016
  52. Jeff KingJun 24, 2016
  53. Junio C HamanoJun 24, 2016
  54. Jeff KingJun 24, 2016
  55. 3/4 archive-tar: write extended headers for far-future mtimeJeff King, Jun 23, 2016
  56. Junio C HamanoJun 24, 2016
  57. Jeff KingJun 24, 2016
  58. 4/4 archive-tar: drop return valueJeff King, Jun 23, 2016
  59. Remi Galan AlfonsoJun 24, 2016
  60. Jeff KingJun 24, 2016
  61. Junio C HamanoJun 24, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.