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

[Pali Rohár]

On Monday 29 October 2018 17:09:58 Chris Lamb wrote:
> Hi Pali,
> 
> Thank you for your detailed and prompt review.
> 
> > 1) Detection of underflow. When you specify negative epoch in env
> > variable, then it underflows and value is converted to some positive
> > integer.
> 
> Getcha, I guess this would be as simple as just another branch:
> 
>  +                    else if (epoch < 0)
>  +                            fprintf(stderr, "SOURCE_DATE_EPOCH must be >= 
> 0\n")

Nope, this is not enough.

You are using strtoull() function which returns unsigned value and does
*not* signal overflow by design. You need to use strtoll() which returns
signed integer and then you can check underflow aby above code.

In my opinion, all function families ato* and strtou*, including
*scanf with numeric formats (%d, %u, ...) should be avoided as they do
not signal invalid input and caller is not able to distinguish between
valid and invalid input. And it is requirement for parsing untrusted
user input.

> > 2) Leading (whitespace) garbage.
> 
> I am not checking this as strtoul(3) and strtoull(3) are "allowed"
> to have such white space:
> 
>     The string may begin with an arbitrary amount of white space
>     (as determined by isspace(3))

And this is my question. It is OK that you accept value which has
leading whitespaces, but do not accept value which has trailing
whitespaces? For me it seems like some inconsistency or something
suspicious. But I'm not maintainer, so I'm just presenting my opinion.

> Whilst it would certianly be possible to check for that I am not
> sure it would be really worth it and add another branch somewhat
> unnecessarily.

Just for inspiration, I used following pattern for converting char *str
of base to integer value.

  long long value;
  char *endptr = NULL;
  errno = 0;
  value = strtoll(str, &endptr, base);
  if (!*str || isspace(*str) || *endptr || errno) {
    /* signal that input str cannot be parsed */
  } else if (value < MY_MIN_CONST || value > MY_MAX_CONST) {
    /* signal that input str is number, but out of range
  } else {
    /* valid input value */
  }

It is pity that such common operation "convert string to number" is hard
to write correctly in C.

> Thanks again for your input.
> 
> 
> Regards,
> 

-- 
Pali Rohár
address@hidden

signature.asc
Description: PGP signature