| Message ID | 20240108173837.20480-2-soekkle@freenet.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mingw: give more details about unsafe directory's | expand |
Sören Krecker <soekkle@freenet.de> writes: > Add domain/username in error message, if owner sid of repository and > user sid are not equal on windows systems. Will queue. "git am --whitespace=fix" noticed numerous whitespace breakages in the patch, which has been corrected on the receiving end. Please make sure your future patches will not have such problems. Thanks. $ git am -s3c ./+sk-v7-mingw-ownership-report Applying: mingw: give more details about unsafe directory's ownership .git/rebase-apply/patch:23: trailing whitespace. &pe_use); .git/rebase-apply/patch:27: trailing whitespace. ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); .git/rebase-apply/patch:45: trailing whitespace. LPSTR str1, str2, str3, str4, to_free1 = NULL, warning: 3 lines add whitespace errors. .git/rebase-apply/patch:23: trailing whitespace. &pe_use); .git/rebase-apply/patch:27: trailing whitespace. ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); .git/rebase-apply/patch:45: trailing whitespace. LPSTR str1, str2, str3, str4, to_free1 = NULL, warning: 3 lines applied after fixing whitespace errors. Using index info to reconstruct a base tree... M compat/mingw.c Falling back to patching base and 3-way merge... Auto-merging compat/mingw.c
Am 08.01.24 um 18:38 schrieb Sören Krecker: > +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). > + /* > + * 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
Johannes Sixt <j6t@kdbg.org> 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@kdbg.org> > > Thank you! > > -- Hannes
Am 09.01.24 um 21:06 schrieb Junio C Hamano: > Johannes Sixt <j6t@kdbg.org> 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@kdbg.org> >> >> Thank you! >> >> -- Hannes -- Hannes
Johannes Sixt <j6t@kdbg.org> writes: >> 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), ... Ah, that is what I missed. Thanks.
diff --git a/compat/mingw.c b/compat/mingw.c index 42053c1f65..d85fae3747 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2684,6 +2684,30 @@ static PSID get_current_user_sid(void) return result; } +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); + /* + * 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); + if (!translate_sid_to_user) + FREE_AND_NULL(*str); + else + (*str)[len_domain] = '/'; + return translate_sid_to_user; +} + static int acls_supported(const char *path) { size_t offset = offset_1st_component(path); @@ -2765,27 +2789,47 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report) strbuf_addf(report, "'%s' is on a file system that does " "not record ownership\n", path); } else if (report) { - LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; + LPSTR str1, str2, str3, str4, to_free1 = NULL, + to_free3 = NULL, to_local_free2 = NULL, + to_local_free4 = NULL; - if (ConvertSidToStringSidA(sid, &str1)) + if (user_sid_to_user_name(sid, &str1)) to_free1 = str1; else str1 = "(inconvertible)"; - - if (!current_user_sid) - str2 = "(none)"; - else if (!IsValidSid(current_user_sid)) - str2 = "(invalid)"; - else if (ConvertSidToStringSidA(current_user_sid, &str2)) - to_free2 = str2; + if (ConvertSidToStringSidA(sid, &str2)) + to_local_free2 = str2; else str2 = "(inconvertible)"; + + if (!current_user_sid) { + str3 = "(none)"; + str4 = "(none)"; + } + else if (!IsValidSid(current_user_sid)) { + str3 = "(invalid)"; + str4 = "(invalid)"; + } else { + if (user_sid_to_user_name(current_user_sid, + &str3)) + to_free3 = str3; + else + str3 = "(inconvertible)"; + if (ConvertSidToStringSidA(current_user_sid, + &str4)) + to_local_free4 = str4; + else + str4 = "(inconvertible)"; + } strbuf_addf(report, "'%s' is owned by:\n" - "\t'%s'\nbut the current user is:\n" - "\t'%s'\n", path, str1, str2); - LocalFree(to_free1); - LocalFree(to_free2); + "\t%s (%s)\nbut the current user is:\n" + "\t%s (%s)\n", + path, str1, str2, str3, str4); + free(to_free1); + LocalFree(to_local_free2); + free(to_free3); + LocalFree(to_local_free4); } }
Add domain/username in error message, if owner sid of repository and user sid are not equal on windows systems. Old error message: ''' fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git' 'C:/Users/test/source/repos/git' is owned by: 'S-1-5-21-571067702-4104414259-3379520149-500' but the current user is: 'S-1-5-21-571067702-4104414259-3379520149-1001' To add an exception for this directory, call: git config --global --add safe.directory C:/Users/test/source/repos/git ''' New error message: ''' fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git' 'C:/Users/test/source/repos/git' is owned by: DESKTOP-L78JVA6/Administrator (S-1-5-21-571067702-4104414259-3379520149-500) but the current user is: DESKTOP-L78JVA6/test (S-1-5-21-571067702-4104414259-3379520149-1001) To add an exception for this directory, call: git config --global --add safe.directory C:/Users/test/source/repos/git ''' Signed-off-by: Sören Krecker <soekkle@freenet.de> --- compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 57 insertions(+), 13 deletions(-)