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. >> + /* >> + * 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 ... >> + 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). > >> + 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. > >> + return translate_sid_to_user; >> +} >> + > > This patch looks good and works for me. > > Acked-by: Johannes Sixt <j6t@xxxxxxxx> > > Thank you! > > -- Hannes
