Re: [PATCH v4 2/5] t5000: test tar files that overflow ustar headers

[Jeff King <peff@peff.net>]

On Thu, Jul 14, 2016 at 01:00:08PM -0700, Junio C Hamano wrote:

> > There's tons of discussion in:
> >
> >   http://thread.gmane.org/gmane.comp.version-control.git/297409
> >
> > but frankly it is not worth your time to read it. These tests are about
> > overflowing the tar limits, which can only happen with times and sizes
> > greater than 32-bits. The right thing to do is to skip the tests
> > entirely on systems where sizeof(unsigned long) is less than 8 (the
> > actual value is 64GB+1, so technically a 37-bit system would work, but I
> > think it is OK for the test-skipping to be less specific).
> 
> OK, how about this on top of a replacement for js/t0006-for-v2.9.2
> that I'll send out as a reply to this message?

Yeah, I think the patch here mostly makes sense. I tried to think what
could go wrong in this hunk:

> diff --git a/archive-tar.c b/archive-tar.c
> index 7ea4e90..4d2832c 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -25,8 +25,13 @@ static int write_tar_filter_archive(const struct archiver *ar,
>   *
>   * Likewise for the mtime (which happens to use a buffer of the same size).
>   */
> +#if ULONG_MAX == 0x7FFFFFFF
> +#define USTAR_MAX_SIZE ULONG_MAX
> +#define USTAR_MAX_MTIME ULONG_MAX
> +#else
>  #define USTAR_MAX_SIZE 077777777777UL
>  #define USTAR_MAX_MTIME 077777777777UL
> +#endif
>  
>  /* writes out the whole block, but only if it is full */
>  static void write_if_needed(void)

If for some reason we are wrong that objects cannot be larger than
ULONG_MAX (e.g., later on we convert everything to size_t, and 64-bit
LLP platforms handle large objects just fine), then we would prematurely
switch to extended headers on those platforms.

I think that's OK. This would just need cleaned up as part of the
conversion from "unsigned long" to "size_t" (the correct check would
then be against the max size_t).

Also, shouldn't it be checking against 0xFFFFFFFF?

An easier check would be "sizeof()", but I guess we can't use that in a
preprocessor directive.

> -test_expect_success TAR_HUGE 'system tar can read our huge size' '
> +test_expect_success TAR_HUGE,64BIT 'system tar can read our huge size' '

The 64BIT prereq is really "unsigned long is 64-bit". I wonder if we
should call it UL64 or something like that to make it more clear.

That makes it unnecessarily tied-in with the implementation, but it does
make it more clear what we care about; the distinction matters for
things like LP64 vs LLP64.

-Peff