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

[Carlo Arenas <carenas@gmail.com>]

On Thu, May 5, 2022 at 7:32 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 05/05/2022 15:01, Johannes Schindelin wrote:
> > [...]
> >> +
> >> +/*
> >> + * this helper function overrides a ROOT_UID with the one provided by
> >> + * an environment variable, do not use unless the original user is
> >> + * root
> >> + */
> >> +static inline void extract_id_from_env(const char *env, uid_t *id)
> >> +{
> >> +    const char *real_uid = getenv(env);
> >> +
> >> +    /* discard any empty values */
> >> +    if (real_uid && *real_uid) {
> >> +            char *endptr;
> >> +            unsigned long env_id;
> >> +            int saved_errno = errno;
> >> +
> >> +            errno = 0;
> >> +            env_id = strtoul(real_uid, &endptr, 10);
> >> +            if (!errno && !*endptr && env_id <= (uid_t)-1)
> >
> > We should not look at `errno` here unless the return value of `strtoul()`
> > indicates that there might have been an error (i.e. when it is
> > `ULONG_MAX`). >
> > Likewise, we need to either initialize `endptr` or only look at it when
> > `strtoul()` succeeded.
>
> I don't think we need to do either of those, and indeed the function you
> suggest below does not do them. The standard guarantees that endptr is
> always set and there is no harm in unconditionally checking errno.

I think the point dscho was trying to make is that while you are
correct that the standard guarantees those two things and
implementation might decide to not do them, we obviously support
systems that are not POSIX.

The irony is that my first confusing attempt to implement this did
that before by explicitly ignoring errno and instead relying in the
"expected overflow/underflow" values to detect the cases we care for
(which is valid UID numbers that are most likely to be uint32_t) and
which was the same thing we got from my original (but hated) atoi.

> > We could side-step all of this, of course, if we simply did this:
> >
> >       euid = getuid();
> >       if (euid == ROOT_UID)
> >               euid = git_env_ulong("SUDO_UID", euid);
>
> That's a nice suggestion, I didn't know that function existed. It means
> we would die() if we could not parse SUDO_UID which I think is
> reasonable (we'd also accept a units suffix an the uid)

which is also why we can't use it, any possibly bogus or suspicious
value we get from SUDO_UID MUST be ignored.

Carlo