Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2024, 20:06 UTC
Message-ID: <xmqq8r4ygtkd.fsf@gitster.g>
In-Reply-To: <d1e1a543-ab9c-4b1b-9f1d-3728e791df2e@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 5 quoted lines

>> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
>> +			  &pe_use); 
>
> At this point, the function fails, so len_user and len_domain contain
> the required buffer size (including the trailing NUL).

So (*str)[len_domain] would be the trailing NUL after the domain part in the next call? Or would that be (*str)[len_domain-1]? I am puzzled by off-by-one with your "including the trailing NUL" remark.

>> +	/*
>> +	 * Alloc needed space of the strings
>> +	 */
>> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);

This obviously assumes for domain 'd' and user 'u', we want "d/u" and len_domain must be 1+1 (including NUL) and len_user must be 1+1 (including NUL). But then ...

>> +	translate_sid_to_user = LookupAccountSidA(NULL, sid,
>> +	    (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);

... ((*str)+len_domain) is presumably the beginning of the user part, and (*str)+0) is where the domain part is to be stored.

Because len_domain includes the terminating NUL for the domain part, (*str)[len_domain-1] is that NUL, no? And that is what you want to overwrite to make the two strings <d> <NUL> <u> <NUL> into a single one <d> <slash> <u> <NUL>. So...

Show 7 quoted lines

> At this point, if the function is successful, len_user and len_domain
> contain the lengths of the names (without the trailing NUL).
>
>> +	if (!translate_sid_to_user)
>> +		FREE_AND_NULL(*str);
>> +	else
>> +		(*str)[len_domain] = '/';

... this offset looks fishy to me.  Am I off-by-one?

Show 18 quoted lines

> Therefore, this overwrites the NUL after the domain name and so
> concatenates the two names. Good.
>
> I found this by dumping the values of the variables, because the
> documentation of LookupAccountSid is not clear about the values that the
> variables receive in the success case.
>
>> +	return translate_sid_to_user;
>> +}
>> +
>
> This patch looks good and works for me.
>
> Acked-by: Johannes Sixt <j6t@kdbg.org>
>
> Thank you!
>
> -- Hannes