Re: [PATCH v5 4/4] git-compat-util: allow root to access both SUDO_UID and root owned

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]

Hi Carlo,

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.

>  {
>  	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.

Ciao,
Dscho