Re: [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged

[Junio C Hamano <gitster@pobox.com>]

Carlo Arenas <carenas@gmail.com> writes:

>> > +     if (real_uid && *real_uid) {
>> > +             char *error;
>> > +             long extracted_id = strtol(real_uid, &error, 10);
>> > +             if (!*error && LONG_MIN < extracted_id &&
>> > +                             extracted_id < LONG_MAX) {
>>
>> strtol() returns a long so the last two checks are redundant.
>
> The last two checks were to check for underflow or overflow and to
> make sure that the bogus values this function returns in case of those
> errors are not taken into consideration.
>
>>The standard is silent on what happens to error when the value is out of
>> range.

Actually the standard is is very clear what happens to endptr (no,
don't call it "error", that is not the point of the parameter).

    A pointer to the final string shall be stored in the object
    pointed to by endptr, provided that endptr is not a null
    pointer.

where "final string" has a precise definition much earlier:

    First, they decompose the input string into three parts:

    1. An initial, possibly empty, sequence of white-space
       characters (as specified by isspace())

    2. A subject sequence interpreted as an integer represented in
       some radix determined by the value of base

    3. A final string of one or more unrecognized characters,
       including the terminating null byte of the input string.

    Then they shall attempt to convert the subject sequence to an
    integer, and return the result.

So, leading whitespace is stripped, then "subject sequence" that is
the sequence of digits (with optional +/- sign) to be turned into a
long is recognised, and what remains is the "final string".  endptr
is made to point at that "final string", and it does not matter what
kind of value the interpretation of "subject sequence" yields.

And that is why it is wrong to call it "error".  It is not about
errors, but is about the shape of the input string, i.e. where the
run of digits ends.

> Which is why instead I was avoiding LONG_{MIN,MAX} which are
> described[1] as the bogus values that will be used in that case

strtol() is designed to return LONG_MIN and LONG_MAX when values
overflow or underflow, but that does not mean it returns these two
values only when the input overflows or underflows.  If it were
strtouint8(), giving it "255" will give 255, while you would get
UCHAR_MAX when the input makes it overflow, e.g. "1000", and from
the returned value you cannot tell which is which because UCHAR_MAX
is 255 ;-)

IOW, you are reading the standard wrong.  It does not say that
LONG_MIN and LONG_MAX are bogus values at all.

And theree is this note:

    Since 0, {LONG_MIN} or {LLONG_MIN}, and {LONG_MAX} or {LLONG_MAX}
    are returned on error and are also valid returns on success, an
    application wishing to check for error situations should set errno
    to 0, then call strtol() or strtoll(), then check errno.

So, no, I do not think the range check gives us anything
meaningful.

At this point, it is tempting to say that it is not worth trying to
do it right by avoiding atoi(), as we are bound to make more of this
kind of mistakes X-<.  But at the same time, I hope we can learn
together and get this right ;-).