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

[<rsbecker@nexbridge.com>]

On April 27, 2022 11:39 AM, Carlo Arenas wrote:
>On Wed, Apr 27, 2022 at 2:33 AM Phillip Wood <phillip.wood123@gmail.com>
>wrote:
>> On 27/04/2022 01:05, Carlo Marcelo Arenas Belón wrote:
>> > diff --git a/git-compat-util.h b/git-compat-util.h index
>> > 58fd813bd01..9bb0eb5087a 100644
>> > --- a/git-compat-util.h
>> > +++ b/git-compat-util.h
>> > @@ -437,12 +437,48 @@ static inline int git_offset_1st_component(const char
>*path)
>> >   #endif
>> >
>> >   #ifndef is_path_owned_by_current_user
>> > +
>> > +#ifdef __TANDEM
>> > +#define ROOT_UID 65535
>> > +#else
>> > +#define ROOT_UID 0
>> > +#endif
>> > +
>> > +/*
>> > + * this helper function overrides a ROOT_UID with the one provided
>> > +by
>> > + * an environment variable, do not use unless the original uid is
>> > + * root
>> > + */
>> > +static inline int extract_id_from_env(const char *env, uid_t *id)
>>
>> Do we really want this living in git-compat-util.h?
>
>No; but IMHO the same applies to is_path_owned_by_current_uid(), and as I
>mentioned in my original proposal refactoring this code to do so has been punted
>until later since the objective here was to keep the change as small as possible for
>clean backporting.
>
>My intention with that comment was not only to warn people that might want to
>reuse that helper but to indicate it was just a hack that should be refactored ASAP.
>
>FWIW, I still think that using atoi with a check to skip "" is probably as safe as doing
>all this extra checking as no one has shown yet a system where sizeof(uid_t) >
>sizeof(uint32_t), but agree with Junio that using long instead avoids issues with
>the systems where
>sizeof(uid_t) > sizeof(int) and unless sizeof(int) == sizeof(long)
>(ex: 32-bit Linux) which is then covered by the cast.
>
>> > +{
>> > +     const char *real_uid = getenv(env);
>> > +
>> > +     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.
>
>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 Agree with you that we could also
>add a check for >=0 as uid_t is usually unsigned, but it is not warranted to be so,
>and that is I was aiming for the wider possible range so we don't have to worry
>that much and let it be settled to a valid value through the cast.

Somehow, the root user id is a compat issue. Perhaps we need a uid_t getRootUid() in git-compat-util.h, and hide the details of the proc from the interface.