[RFC PATCH v4 0/3] fix `sudo make install` regression in maint

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



A reroll for cb/path-owner-check-with-sudo with most of the suggestions
included but planned originally as an RFC, because I am frankly not sure
that I read and addressed all of it, but also because after seeing how
my only chance to get an official Reviewed-by: Junio vanished I also
realized that I wasn't been clear enough and careful enough from the
beginning to explain the code correctly and therefore had maybe wasted
more of the review quota this change should require.

Important changes (eventhough most are not affecting the logic)
* Document hopefully better which environments are supported and what
  to do if you want to test it in one that is not (thanks to dscho)
* Removed the arguably premature optimization to try to keep the sudo
  cache warm which was actually buggy, as it was also not needed.
  The CI does passwordless sudo unconditionally and even when running
  it locally it will go so fast that is shouldn't be an issue (thanks
  to Philip)
* No longer using the ugly and controversial variable name so now it
  will need GIT_TEST_ENABLE_SUDO to be used to enable it on CI (which
  is not done in this series, but a run with it enabled on top of
  seen is available[1])
* Stop the arguing about what is or not a regression worth fixing and
  instead document it as one through a test, which would be easy to
  fix in a follow up since the code was already provided by Junio

Lastly I am little concerned by the fact this is going to maint but
has a "weather balloon" of sorts, which might not be wise, since it
might prevent people that might be affected from upgrading if they
have a -Werror policy.

The effect is minor though, as worst case, if someone has a system
with a signed uid_t then this "feature" wouldn't work for them and
nothing has changed but I think it is worth to consider the alternatives
which are (in my own order of preference)

* Revert the change to use unsigned long and strtoul()

  This will mean that people running in a 32bit system with an uid bigger
  than INT_MAX wouldn't be able to use the feature

* Move the code out (which is indeed an artificial restriction) so that
  we can use intmax_t and strtoimax() instead and a cast to compare the
  uid_t.

  This avoids all issues and restrictions but means more code changes

* Throw away the custom function and expand the API ones to be used
  instead as dscho suggested.

  Even more code changes, but maybe less risk as we will be building
  on top of battle tested code.

[1] https://github.com/carenas/git/actions/runs/2286452160

Carlo Marcelo Arenas Belón (3):
  t: regression git needs safe.directory when using sudo
  git-compat-util: avoid failing dir ownership checks if running
    privileged
  t0034: add negative tests and allow git init to mostly work under sudo

 Documentation/config/safe.txt  |  10 ++++
 git-compat-util.h              |  49 +++++++++++++++-
 t/lib-sudo.sh                  |  12 ++++
 t/t0034-root-safe-directory.sh | 103 +++++++++++++++++++++++++++++++++
 4 files changed, 173 insertions(+), 1 deletion(-)
 create mode 100644 t/lib-sudo.sh
 create mode 100755 t/t0034-root-safe-directory.sh

-- 
2.36.1.371.g0fb0ef0c8d




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux