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

From: Johannes Sixt <j6t@kdbg.org>
Date: Jan 9, 2024, 21:05 UTC
Message-ID: <e7594386-4a26-467d-bd27-8ac6268ad219@kdbg.org>
In-Reply-To: <xmqq8r4ygtkd.fsf@gitster.g>

Am 09.01.24 um 21:06 schrieb Junio C Hamano:

Show 11 quoted lines

> Johannes Sixt <j6t@kdbg.org> writes:
> 
>>> +	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.

"Required buffer size" must count the trailing NUL. So, the NUL would be at (*str)[len_domain-1].

Show 9 quoted lines

> 
>>> +	/*
>>> +	 * 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 ...

So, this allocates the exact amount that is required to contain the names with the trailing NULs: 1+1 plus 1+1 in this example.

Show 6 quoted lines

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

Correct.

Show 5 quoted lines

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

But after a successful call, len_domain and len_user have been modified to contain the lengths of the names (not counting the NULs), so, here the NUL is at (*str)[len_domain]...

Show 10 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?

... and this offset is correct.

I followed the same train of thought and suspected an off-by-one error, too, and was perplexed that I see a correct output. The documentation of LookupAccountSid is unclear that the variables change values across the (successful) call, but my tests verified that the change does happen.

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

-- Hannes