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

From: Johannes Sixt <j6t@kdbg.org>
Date: Jan 9, 2024, 19:27 UTC
Message-ID: <d1e1a543-ab9c-4b1b-9f1d-3728e791df2e@kdbg.org>
In-Reply-To: <20240108173837.20480-2-soekkle@freenet.de>

Am 08.01.24 um 18:38 schrieb Sören Krecker:

Show 11 quoted lines

> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/*
> +	 * returns only FALSE, because the string pointers are NULL
> +	 */
> +	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).

Show 6 quoted lines

> +	/*
> +	 * Alloc needed space of the strings
> +	 */
> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid,
> +	    (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);

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] = '/';

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