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

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

 



Am 09.01.24 um 21:06 schrieb Junio C Hamano:
> Johannes Sixt <j6t@xxxxxxxx> 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].

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

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

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

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

>> 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@xxxxxxxx>
>>
>> Thank you!
>>
>> -- Hannes

-- Hannes





[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