On Wed, Jun 15, 2022 at 7:03 AM Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
>
> On Thu, 12 May 2022, Carlo Marcelo Arenas Belón wrote:
>
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index e7cbfa65c9a..0a5a4ee7a9a 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -420,9 +420,10 @@ static inline int git_offset_1st_component(const char *path)
> > * maybe provide you with a patch that would prevent this issue again
> > * in the future.
> > */
> > -static inline void extract_id_from_env(const char *env, uid_t *id)
> > +static inline int id_from_env_matches(const char *env, uid_t id)
>
> I agree somewhat with Gabór's concern that this patch tries to do too many
> things at once, including this rename.
>
> We have a recent history of introducing so many regressions that `.1`
> releases have become the norm rather than the exception, and from where I
> sit, the reason is squarely with the uptick in refactoring (including
> renames like this one).
>
> So unless the refactoring is done to any other end than for refactoring's
> own sake (which is really not a good reason), I see it as problematic.
I couldn't agree more with that sentiment, but in this case the
refactoring was done to clean up the recently introduced function
which was indeed too ugly (some would say on purpose) and replacing it
with not only better looking code, but also a cleaner interface.
I would think that in this specific case an exemption could be
granted, but it is also true that while this code is almost a month
old, it hasn't got the review it will require to be merged so late in
the release cycle without raising the concerns you both fairly put
forward.
> > {
> > const char *real_uid = getenv(env);
> > + int matches = 0;
> >
> > /* discard anything empty to avoid a more complex check below */
> > if (real_uid && *real_uid) {
> > @@ -432,9 +433,10 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
> > errno = 0;
> > /* silent overflow errors could trigger a bug here */
> > env_id = strtoul(real_uid, &endptr, 10);
> > - if (!*endptr && !errno)
> > - *id = env_id;
> > + if (!*endptr && !errno && (uid_t)env_id == id)
> > + matches = 1;
> > }
> > + return matches;
> > }
> >
> > static inline int is_path_owned_by_current_uid(const char *path)
> > @@ -446,10 +448,13 @@ static inline int is_path_owned_by_current_uid(const char *path)
> > return 0;
> >
> > euid = geteuid();
> > + if (st.st_uid == euid)
> > + return 1;
> > +
> > if (euid == ROOT_UID)
> > - extract_id_from_env("SUDO_UID", &euid);
> > + return id_from_env_matches("SUDO_UID", st.st_uid);
>
> A much shorter, much more obvious patch would look like this:
>
> - if (euid == ROOT_UID)
> + if (st.st_uid != euid && euid == ROOT_UID && )
> extract_id_from_env("SUDO_UID", &euid);
>
> It accomplishes the same goal, but is eminently easier to review. For
> regression fixes, I much prefer the safety and confidence that comes with
> that.
will reroll with your suggestion, thanks again for your helpful
review, and apologies again to everyone for not cleaning it up earlier
in the cycle.
IMHO the alternative (which will be releasing with the regression)
might be worse than releasing with this version so we ought to do
something before RC1.
Carlo
Carlo
