Re: [Info-mtools] [PATCH] misc.c: Make the output reproducible

[Pali Rohár]

On Monday 29 October 2018 16:45:20 Chris Lamb wrote:
> Hi mtools maintainers,
> 
> Whilst working on the Reproducible Builds effort [0], we noticed
> that mtools generates non-reproducible output.
> 
> This is because it uses the current time of day as a default
> timestamp, such as when adding files to an existing file.
> 
> Patch attached that uses the SOURCE_DATE_EPOCH [1] environment
> variables. It was initially filed in Debian bug #900410 [2]) and has
> been incorporated into the version distributed by that operating
> system seemingly without issue.

Hi Chris! I'm not maintainer, but I spotted bug in your patch...

> @@ -159,11 +160,33 @@ void print_sector(const char *message, unsigned char 
> *data, int size)
>  
>  time_t getTimeNow(time_t *now)
>  {
> +     char *endptr;
> +     char *source_date_epoch;
> +     unsigned long long epoch;
>       static int haveTime = 0;
>       static time_t sharedNow;
>  
>       if(!haveTime) {
> -             time(&sharedNow);
> +             source_date_epoch = getenv("SOURCE_DATE_EPOCH");
> +             if (source_date_epoch) {
> +                     epoch = strtoull(source_date_epoch, &endptr, 10);
> +
> +                     if (endptr == source_date_epoch)
> +                             fprintf(stderr, "SOURCE_DATE_EPOCH invalid\n");
> +                     else if ((errno == ERANGE && (epoch == ULLONG_MAX || 
> epoch == 0))
> +                                     || (errno != 0 && epoch == 0))
> +                             fprintf(stderr, "SOURCE_DATE_EPOCH: strtoull: 
> %s: %llu\n",
> +                             strerror(errno), epoch);
> +                     else if (*endptr != '\0')
> +                             fprintf(stderr, "SOURCE_DATE_EPOCH has trailing 
> garbage\n");
> +                     else if (epoch > ULONG_MAX)
> +                             fprintf(stderr, "SOURCE_DATE_EPOCH must be <= 
> %lu but saw: %llu\n", ULONG_MAX, epoch);

Above code does not check for two things:

1) Detection of underflow. When you specify negative epoch in env
variable, then it underflows and value is converted to some positive
integer.

2) Leading (whitespace) garbage. You are already checking that env
variable does not have trialing garbage (including trailing whitespaces)
but you are missing check that env variable does not have leading
whitespaces.

> +                     else {
> +                             sharedNow = epoch;
> +                     }
> +             } else {
> +                     time(&sharedNow);
> +             }
>               haveTime = 1;
>       }
>       if(now)

-- 
Pali Rohár
address@hidden

signature.asc
Description: PGP signature