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

From: Jeff King <peff@peff.net>
Date: Jul 14, 2016, 20:10 UTC
Message-ID: <20160714201018.GA18372@sigill.intra.peff.net>
In-Reply-To: <xmqqshvcourb.fsf@gitster.mtv.corp.google.com>

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

Show 13 quoted lines

> > 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:

Show 18 quoted lines

> 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