Am 09.01.24 um 21:06 schrieb Junio C Hamano:

Johannes Sixt [off-list ref] writes:

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

At this point, if the function is successful, len_user and len_domain
contain the lengths of the names (without the trailing NUL).

quoted

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

quoted

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.

quoted

+	return translate_sid_to_user;
+}
+

This patch looks good and works for me.

Acked-by: Johannes Sixt <redacted>

Thank you!

-- Hannes

-- Hannes