On Wed, Apr 27, 2022 at 2:33 AM Phillip Wood <phillip.wood123@xxxxxxxxx> 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.
Carlo
[1] https://pubs.opengroup.org/onlinepubs/007904875/functions/strtol.html
