Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo

[Carlo Arenas <carenas@gmail.com>]

On Fri, May 6, 2022 at 2:43 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Arenas <carenas@gmail.com> writes:
>
> > Since I am renaming it anyway as part of this topic with RFC v4, would
> > it be a good idea to require both?
> >
> > I see the "IKNOWHATIAMDOING" not as a normal GIT_TEST flag, but as a
> > "here be dragons!" warning, and I later found that I either
> > misremembered it being enabled in the CI, or it was dropped with one
> > of those refactors we do often there.
> >
> > My RFC v4 includes a new nice looking GIT_TEST variable as suggested
> > by Phillip which I am also enabling in the CI to hopefully make it
> > even more clear that this is only meant to run there, but sadly that
> > also means that this patch will likely have a conflict when merged
> > upwards.
>
> This must build from the older mainteance tracks like maint-2.30, so
> let's keep the changes to absolute minimum

makes sense, but I still unsure about the two questions I had above:

* would it make sense to make it run ONLY if both variables are set to
YES or is it enough to use one (most likely the GIT_TEST one) that we
would enable independently in CI?

the advantage I see of having both variables is that it is even less
likely to even run accidentally in a desktop somewhere and maybe break
that test, and also keeps the meaning I wanted to attach to it with
that ugly looking flag that no one should ever try to enable in their
workstations unless they really know what they are doing.

The advantage of ONLY having the GIT_TEST one is that it will be
easier to enable in CI and for whoever wants to play with it on their
workstation as well, but might still encourage people trying to make
it work and wasting their time.

* since we have to enable CI for these to be useful, would that be
something to be done in an additional patch as part of this topic
branch and you will only pull the commits before it to maint to avoid
conflicts, or should it be done completely independently as a mini
feature branch that depends on this one that will be pulled to seen
and merged downwards from it?

somehow offtopic but just curious about your process, presume that if
we go with a single topic branch adding it instead as 2/4 would break
your flow/scripts since the only way to get that merged would be to
cherry-pick, right?

> I actually think 1/3 and 3/3 are OK.  Are there remaining issues in
> these two patches (which only are tests)?

The versions of them in RFCv4 have more documentation and are cleaner
since they mostly include most of the feedback that was collected on
them (even if I am still unsure because it is spread around and
difficult to track, hence why I was planning an RFC)

I don't think there is anything significantly different though but
they are and will need another review (which I am hoping would be
uncontroversial)

> As to 2/3, I think the code is basically already fine, but a
> simplification like the following on top would be a good idea.
>
>  * We clear errno before making strtoul() call, so any non-zero
>    errno must have happeneed in strtoul(), which includes ERANGE.
>    There is no point checking the returned value env_id; if it is
>    ULONG_MAX but errno is 0, then the SUDO_UID legitimately is
>    naming a user whose UID is that special value, and it is not an
>    indication of an overflow.

true, but the apparent check for ULONG_MAX (which should have a
comment added) was really a check not to overflow when assigning the
value we got into uid_t, by sacrificing an unlikely to be valid
ULONG_MAX as an uid.

it also has the intentional side effect of breaking this code if uid_t
is signed and hopefully triggering a warning in the process since it
would be always false, that way whoever has a system where this type
is signed will have to carry their own version of the code and we
don't have to deal with the portability of it.

lastly (since it really made me proud and would be sad to see it go)
it ALSO avoids someone trying to sneak a value that would overflow in
one of the most common configurations we will run where sizeof(long) >
sizeof(uid_t) && MIN_UID_T >=0, by using an equivalent to MAX_UID_T
(which only exists in a few systems and therefore can't be relied on)
to discard values that would overflow the range uid_t has.

without it, we would probably find ourselves in the future having to
deal with an embarrassing bug that others before us had suffered.

Carlo