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

[Pali Rohár]

Hi Chris!

This code is still incorrect for 64 bit architectures, like amd64.

On Tuesday 30 October 2018 00:34:59 Chris Lamb wrote:
> @@ -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;

Variable epoch is unsigned.

>       static int haveTime = 0;
>       static time_t sharedNow;
>  
>       if(!haveTime) {
> -             time(&sharedNow);
> +             source_date_epoch = getenv("SOURCE_DATE_EPOCH");
> +             if (source_date_epoch) {
> +                     epoch = strtoll(source_date_epoch, &endptr, 10);

But converted value is signed (possible negative). So you are casting
here negative value into two-complement positive number.

> +                     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: strtoll: 
> %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);

And here you are doing check which is always false for 64 bit
architectures, like amd64. On amd64 are "long long" and "long" types of
same size, both 64 bit. Therefore check if epoch is larger then maximal
unsigned 64bit value is always false as epoch can be maximally 64 bit
value.

You can recheck that "unsigned long" and "unsigned long long" are same:

  #include <stdio.h>
  #include <limits.h>
  int main() {
    printf("%lu\n%llu\n", ULONG_MAX, ULLONG_MAX);
    return 0;
  }

$ gcc -m64 test.c
$ ./a.out
18446744073709551615
18446744073709551615

> +                     else {
> +                             sharedNow = epoch;

And here you are assigning epoch into sharedNow variable which is of
type time_t. You need to check for overflow/underflow before assigning.

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

Here is (untested) alternative:

        char *endptr;
        char *sourceDateEpoch;
        long long epoch;
        static int haveTime = 0;
        static time_t sharedNow;

        if (!haveTime) {
                sourceDateEpoch = getenv("SOURCE_DATE_EPOCH");
                if (sourceDateEpoch) {
                        errno = 0;
                        epoch = strtoll(sourceDateEpoch, &endptr, 10);
                        if (!*sourceDateEpoch || isspace(*sourceDateEpoch) || 
*endptr || (errno && errno != ERANGE)) {
                                fprintf(stderr, "SOURCE_DATE_EPOCH invalid\n");
                                sharedNow = -1;
                        } else {
                                sharedNow = epoch;
                                if (((time_t)-1 > 0 && epoch < 0) || sharedNow 
> LLONG_MAX || (long long)sharedNow != epoch || errno == ERANGE) {
                                        fprintf(stderr, "SOURCE_DATE_EPOCH is 
out or range\n");
                                        sharedNow = -1;
                                }
                        }
                } else {
                        time(&sharedNow);
                }
                haveTime = 1;
        }

It uses correct signed long long time with strtoll() funcion. It
properly erases errno to zero prior strtoll() call.

When SOURCE_DATE_EPOCH is invalid or out of range, then sharedNow is set
to -1. Same what does time() on error.

Out or range error message is printed when either strtoll signals out of
range or when converted value is out of range. This simplify lot of
if-else branches.

Plus I added there simple check for overflow/underflow when casting from
"long long" to "time_t". IIRC there is no constant for TIME_MAX/TIME_MIN
and because time_t can be signed, unsigned, 32 bit or 64 bit, it may
overflow too...

-- 
Pali Rohár
address@hidden