Johannes Sixt [off-list ref] writes:

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.

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

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.

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

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?

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