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
