Re: [PATCH v3 2/3] git-compat-util: avoid failing dir ownership checks if running privileged

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Dscho

On 05/05/2022 15:01, Johannes Schindelin wrote:
> [...]
>> +
>> +/*
>> + * this helper function overrides a ROOT_UID with the one provided by
>> + * an environment variable, do not use unless the original user is
>> + * root
>> + */
>> +static inline void extract_id_from_env(const char *env, uid_t *id)
>> +{
>> +	const char *real_uid = getenv(env);
>> +
>> +	/* discard any empty values */
>> +	if (real_uid && *real_uid) {
>> +		char *endptr;
>> +		unsigned long env_id;
>> +		int saved_errno = errno;
>> +
>> +		errno = 0;
>> +		env_id = strtoul(real_uid, &endptr, 10);
>> +		if (!errno && !*endptr && env_id <= (uid_t)-1)
> 
> We should not look at `errno` here unless the return value of `strtoul()`
> indicates that there might have been an error (i.e. when it is
> `ULONG_MAX`). >
> Likewise, we need to either initialize `endptr` or only look at it when
> `strtoul()` succeeded.

I don't think we need to do either of those, and indeed the function you 
suggest below does not do them. The standard guarantees that endptr is 
always set and there is no harm in unconditionally checking errno.

> We could side-step all of this, of course, if we simply did this:
> 
> 	euid = getuid();
> 	if (euid == ROOT_UID)
> 		euid = git_env_ulong("SUDO_UID", euid);

That's a nice suggestion, I didn't know that function existed. It means 
we would die() if we could not parse SUDO_UID which I think is 
reasonable (we'd also accept a units suffix an the uid)

Best Wishes

Phillip