{"thread":{"id":"60669","subject":"[PATCH 0/1 v2] Replace SID with domain/username on Windows","startedAt":"2023-12-29T12:03:49Z","lastAt":"2024-01-09T22:34:43Z","messageCount":28,"participants":["Sören Krecker","Eric Sunshine","Junio C Hamano","Matthias Aßhauer","Johannes Sixt","Dragan Simic"],"isPatch":true,"patchVersion":2,"patchTotal":1},"messages":[{"id":"486162","messageId":"20231229120319.3797-1-soekkle@freenet.de","threadId":"60669","inReplyTo":null,"subject":"[PATCH 0/1 v2] Replace SID with domain/username on Windows","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2023-12-29T12:03:18Z","receivedAt":"2023-12-29T12:03:49Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Improve error message on windows systems, if owner of reposotory and current user are not equal. \n\n\nOld Message:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n\t'S-1-5-21-571067702-4104414259-3379520149-500'\nbut the current user is:\n\t'S-1-5-21-571067702-4104414259-3379520149-1001'\nTo add an exception for this directory, call:\n\n\tgit config --global --add safe.directory C:/Users/test/source/repos/git \n'''\n\nNew Massage:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/soren/source/repos/git' is owned by:\n 'DESKTOP-L78JVA6/Administrator'\nbut the current user is:\n 'DESKTOP-L78JVA6/test'\nTo add an exception for this directory, call:\n\n git config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\n\nI hope that I have succeeded in addressing all the points raised.\n\nSören Krecker (1):\n Replace SID with domain/username\n\n compat/mingw.c | 28 ++++++++++++++++++++++++----\n 1 file changed, 24 insertions(+), 4 deletions(-)\n\n\nbase-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n-- \n2.39.2\n\n"},{"id":"486163","messageId":"20231229120319.3797-2-soekkle@freenet.de","threadId":"60669","inReplyTo":"20231229120319.3797-1-soekkle@freenet.de","subject":"[PATCH v2 1/1] Replace SID with domain/username","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2023-12-29T12:03:19Z","receivedAt":"2023-12-29T12:03:51Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Replace SID with domain/username in erromessage, if owner of repository\nand user are not equal on windows systems.\n\nSigned-off-by: Sören Krecker \n---\n compat/mingw.c | 28 ++++++++++++++++++++++++----\n 1 file changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 42053c1f65..05aeaaa9ad 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n+{\n+\tSID_NAME_USE pe_use;\n+\tDWORD len_user = 0, len_domain = 0;\n+\tBOOL translate_sid_to_user;\n+\n+\t/* returns only FALSE, because the string pointers are NULL*/\n+\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n+\t\t\t &pe_use); \n+\t/*Alloc needed space of the strings*/\n+\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n+\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n+\t\t\t\t *str, &len_domain, &pe_use);\n+\t*(*str + len_domain) = '/';\n+\tif (translate_sid_to_user == FALSE) {\n+\t\tFREE_AND_NULL(*str);\n+\t}\n+\treturn translate_sid_to_user;\n+}\n+\n static int acls_supported(const char *path)\n {\n \tsize_t offset = offset_1st_component(path);\n@@ -2767,7 +2787,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t} else if (report) {\n \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n \n-\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n+\t\t\tif (user_sid_to_user_name(sid, &str1))\n \t\t\t\tto_free1 = str1;\n \t\t\telse\n \t\t\t\tstr1 = \"(inconvertible)\";\n@@ -2776,7 +2796,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\t\tstr2 = \"(none)\";\n \t\t\telse if (!IsValidSid(current_user_sid))\n \t\t\t\tstr2 = \"(invalid)\";\n-\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n+\t\t\telse if (user_sid_to_user_name(current_user_sid, &str2))\n \t\t\t\tto_free2 = str2;\n \t\t\telse\n \t\t\t\tstr2 = \"(inconvertible)\";\n@@ -2784,8 +2804,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\t\t \"'%s' is owned by:\\n\"\n \t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n \t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n-\t\t\tLocalFree(to_free1);\n-\t\t\tLocalFree(to_free2);\n+\t\t\tfree(to_free1);\n+\t\t\tfree(to_free2);\n \t\t}\n \t}\n \n-- \n2.39.2\n\n"},{"id":"486172","messageId":"CAPig+cT4jy4MkyGxtSOZj6U3vUxLaRa-4wr7PON-EebAjT8pwQ@mail.gmail.com","threadId":"60669","inReplyTo":"20231229120319.3797-1-soekkle@freenet.de","subject":"Re: [PATCH 0/1 v2] Replace SID with domain/username on Windows","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-12-31T04:08:51Z","receivedAt":"2023-12-31T04:09:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 29, 2023 at 7:03 AM Sören Krecker wrote:\n> Improve error message on windows systems, if owner of reposotory and current user are not equal.\n>\n> Old Message:\n> '''\n> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n> 'C:/Users/test/source/repos/git' is owned by:\n> 'S-1-5-21-571067702-4104414259-3379520149-500'\n> but the current user is:\n> 'S-1-5-21-571067702-4104414259-3379520149-1001'\n> To add an exception for this directory, call:\n>\n> git config --global --add safe.directory C:/Users/test/source/repos/git\n> '''\n>\n> New Massage:\n> '''\n> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n> 'C:/Users/soren/source/repos/git' is owned by:\n> 'DESKTOP-L78JVA6/Administrator'\n> but the current user is:\n> 'DESKTOP-L78JVA6/test'\n> To add an exception for this directory, call:\n>\n> git config --global --add safe.directory C:/Users/test/source/repos/git\n> '''\n>\n> I hope that I have succeeded in addressing all the points raised.\n\nThanks, this explanation does an excellent job of helping reviewers\nunderstand why the patch is desirable.\n\nIt's also important that people digging through the project history in\nthe future also understand the reason for this change, so it's very\nvaluable for the explanation to be part of the commit message of the\npatch itself, not just in the cover letter (which doesn't become part\nof the permanent project history). Therefore, can you reroll once\nagain, placing this explanation in the commit message of the patch\nitself? Doing so should help the patch get accepted into the project.\n\nThat's as much as I can add. Hopefully, one or more Windows folks will\nchime in regarding the actual patch content (whether it's acceptable\nas-is or needs some tweaks).\n\nThanks.\n"},{"id":"486173","messageId":"20231231091245.2853-1-soekkle@freenet.de","threadId":"60669","inReplyTo":"CAPig+cT4jy4MkyGxtSOZj6U3vUxLaRa-4wr7PON-EebAjT8pwQ@mail.gmail.com","subject":"[PATCH V3 0/1] Replace SID with domain/username on Windows","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2023-12-31T09:12:44Z","receivedAt":"2023-12-31T09:13:00Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"I improve the commit message with example of old and new error output.\n\nThanks to Eric Sunshine.\n\nSören Krecker (1):\n Replace SID with domain/username\n\n compat/mingw.c | 28 ++++++++++++++++++++++++----\n 1 file changed, 24 insertions(+), 4 deletions(-)\n\n\nbase-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n-- \n2.39.2\n\n"},{"id":"486174","messageId":"20231231091245.2853-2-soekkle@freenet.de","threadId":"60669","inReplyTo":"20231231091245.2853-1-soekkle@freenet.de","subject":"[PATCH v3 1/1] Replace SID with domain/username","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2023-12-31T09:12:45Z","receivedAt":"2023-12-31T09:13:02Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Replace SID with domain/username in error message, if owner of repository\nand user are not equal on windows systems.\n\nOld message:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n\t'S-1-5-21-571067702-4104414259-3379520149-500'\nbut the current user is:\n\t'S-1-5-21-571067702-4104414259-3379520149-1001'\nTo add an exception for this directory, call:\n\n\tgit config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nNew massage:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n 'DESKTOP-L78JVA6/Administrator'\nbut the current user is:\n 'DESKTOP-L78JVA6/test'\nTo add an exception for this directory, call:\n\n git config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nSigned-off-by: Sören Krecker \n---\n compat/mingw.c | 28 ++++++++++++++++++++++++----\n 1 file changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 42053c1f65..05aeaaa9ad 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n+{\n+\tSID_NAME_USE pe_use;\n+\tDWORD len_user = 0, len_domain = 0;\n+\tBOOL translate_sid_to_user;\n+\n+\t/* returns only FALSE, because the string pointers are NULL*/\n+\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n+\t\t\t &pe_use); \n+\t/*Alloc needed space of the strings*/\n+\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n+\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n+\t\t\t\t *str, &len_domain, &pe_use);\n+\t*(*str + len_domain) = '/';\n+\tif (translate_sid_to_user == FALSE) {\n+\t\tFREE_AND_NULL(*str);\n+\t}\n+\treturn translate_sid_to_user;\n+}\n+\n static int acls_supported(const char *path)\n {\n \tsize_t offset = offset_1st_component(path);\n@@ -2767,7 +2787,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t} else if (report) {\n \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n \n-\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n+\t\t\tif (user_sid_to_user_name(sid, &str1))\n \t\t\t\tto_free1 = str1;\n \t\t\telse\n \t\t\t\tstr1 = \"(inconvertible)\";\n@@ -2776,7 +2796,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\t\tstr2 = \"(none)\";\n \t\t\telse if (!IsValidSid(current_user_sid))\n \t\t\t\tstr2 = \"(invalid)\";\n-\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n+\t\t\telse if (user_sid_to_user_name(current_user_sid, &str2))\n \t\t\t\tto_free2 = str2;\n \t\t\telse\n \t\t\t\tstr2 = \"(inconvertible)\";\n@@ -2784,8 +2804,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\t\t \"'%s' is owned by:\\n\"\n \t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n \t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n-\t\t\tLocalFree(to_free1);\n-\t\t\tLocalFree(to_free2);\n+\t\t\tfree(to_free1);\n+\t\t\tfree(to_free2);\n \t\t}\n \t}\n \n-- \n2.39.2\n\n"},{"id":"486175","messageId":"CAPig+cQfwOTRubBDoXjinDrVGnRSXqrt48vsNqv2gKDC=9ep_w@mail.gmail.com","threadId":"60669","inReplyTo":"20231231091245.2853-1-soekkle@freenet.de","subject":"Re: [PATCH V3 0/1] Replace SID with domain/username on Windows","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-12-31T09:18:59Z","receivedAt":"2023-12-31T09:19:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 31, 2023 at 4:12 AM Sören Krecker wrote:\n> I improve the commit message with example of old and new error output.\n>\n> Thanks to Eric Sunshine.\n\nThanks for rerolling.\n"},{"id":"486197","messageId":"xmqqplyjg10l.fsf@gitster.g","threadId":"60669","inReplyTo":"20231229120319.3797-2-soekkle@freenet.de","subject":"Re: [PATCH v2 1/1] Replace SID with domain/username","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-02T16:20:42Z","receivedAt":"2024-01-02T16:20:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> Replace SID with domain/username in erromessage, if owner of repository\n> and user are not equal on windows systems.\n\n\"erromessage\" -> \"error messages\" or something?\n\nThis may not be a question raised by anybody who know Windows, but\nbecause I do not do Windows, it makes me wonder if this is losing\ninformation. Can two SID for the same user be active at the same\ntime, which would cause user_sid_to_user_name() potentially yield\nthe same string for two different SID?\n\nIn any case, I am reasonably sure that Dscho will say yes or no to\nthis patch (the above \"makes me wonder\" does not need to be\nresolved) and I can wait until then.\n\nThanks.\n\n> Signed-off-by: Sören Krecker \n> ---\n> compat/mingw.c | 28 ++++++++++++++++++++++++----\n> 1 file changed, 24 insertions(+), 4 deletions(-)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 42053c1f65..05aeaaa9ad 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)\n> \treturn result;\n> }\n> \n> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n> +{\n> +\tSID_NAME_USE pe_use;\n> +\tDWORD len_user = 0, len_domain = 0;\n> +\tBOOL translate_sid_to_user;\n> +\n> +\t/* returns only FALSE, because the string pointers are NULL*/\n> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n> +\t\t\t &pe_use); \n> +\t/*Alloc needed space of the strings*/\n> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n> +\t\t\t\t *str, &len_domain, &pe_use);\n> +\t*(*str + len_domain) = '/';\n> +\tif (translate_sid_to_user == FALSE) {\n> +\t\tFREE_AND_NULL(*str);\n> +\t}\n> +\treturn translate_sid_to_user;\n> +}\n> +\n> static int acls_supported(const char *path)\n> {\n> \tsize_t offset = offset_1st_component(path);\n> @@ -2767,7 +2787,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n> \t\t} else if (report) {\n> \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n> \n> -\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n> +\t\t\tif (user_sid_to_user_name(sid, &str1))\n> \t\t\t\tto_free1 = str1;\n> \t\t\telse\n> \t\t\t\tstr1 = \"(inconvertible)\";\n> @@ -2776,7 +2796,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n> \t\t\t\tstr2 = \"(none)\";\n> \t\t\telse if (!IsValidSid(current_user_sid))\n> \t\t\t\tstr2 = \"(invalid)\";\n> -\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n> +\t\t\telse if (user_sid_to_user_name(current_user_sid, &str2))\n> \t\t\t\tto_free2 = str2;\n> \t\t\telse\n> \t\t\t\tstr2 = \"(inconvertible)\";\n> @@ -2784,8 +2804,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n> \t\t\t\t \"'%s' is owned by:\\n\"\n> \t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n> \t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n> -\t\t\tLocalFree(to_free1);\n> -\t\t\tLocalFree(to_free2);\n> +\t\t\tfree(to_free1);\n> +\t\t\tfree(to_free2);\n> \t\t}\n> \t}\n"},{"id":"486201","messageId":"xmqqy1d7ej3d.fsf@gitster.g","threadId":"60669","inReplyTo":"xmqqplyjg10l.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] Replace SID with domain/username","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-02T17:33:10Z","receivedAt":"2024-01-02T17:33:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Sören Krecker writes:\n>\n>> Replace SID with domain/username in erromessage, if owner of repository\n>> and user are not equal on windows systems.\n>\n> \"erromessage\" -> \"error messages\" or something?\n>\n> This may not be a question raised by anybody who know Windows, but\n> because I do not do Windows, it makes me wonder if this is losing\n> information. Can two SID for the same user be active at the same\n> time, which would cause user_sid_to_user_name() potentially yield\n> the same string for two different SID?\n>\n> In any case, I am reasonably sure that Dscho will say yes or no to\n> this patch (the above \"makes me wonder\" does not need to be\n> resolved) and I can wait until then.\n>\n> Thanks.\n\nAnother thing I forgot to mention (but did wonder). The new helper\nfunction does allow LookupAccountSidA() to fail. Should it fall\nback to ConvertSidToStringSidA() that the original has been using?\n\nIn any case, I do not think a failure to convert will result in an\nattempt to format (\"%s\", NULL) thanks to the existing code that uses\nthe stringified SID, which is good.\n"},{"id":"486202","messageId":"xmqqsf3feilq.fsf@gitster.g","threadId":"60669","inReplyTo":"20231231091245.2853-2-soekkle@freenet.de","subject":"Re: [PATCH v3 1/1] Replace SID with domain/username","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-02T17:43:45Z","receivedAt":"2024-01-02T17:43:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> Replace SID with domain/username in error message, if owner of repository\n> and user are not equal on windows systems.\n>\n> Old message:\n> '''\n> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n> 'C:/Users/test/source/repos/git' is owned by:\n> \t'S-1-5-21-571067702-4104414259-3379520149-500'\n> but the current user is:\n> \t'S-1-5-21-571067702-4104414259-3379520149-1001'\n> To add an exception for this directory, call:\n>\n> \tgit config --global --add safe.directory C:/Users/test/source/repos/git\n> '''\n>\n> New massage:\n\n\"massage\"???\n\n> '''\n> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n> 'C:/Users/test/source/repos/git' is owned by:\n> 'DESKTOP-L78JVA6/Administrator'\n> but the current user is:\n> 'DESKTOP-L78JVA6/test'\n> To add an exception for this directory, call:\n>\n> git config --global --add safe.directory C:/Users/test/source/repos/git\n> '''\n\nThe same \"does this lose information?\" comment applies to this one\nas well, as the fundamental approach is unchanged.\n\n> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n> +{\n> +\tSID_NAME_USE pe_use;\n> +\tDWORD len_user = 0, len_domain = 0;\n> +\tBOOL translate_sid_to_user;\n> +\n> +\t/* returns only FALSE, because the string pointers are NULL*/\n> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n> +\t\t\t &pe_use); \n> +\t/*Alloc needed space of the strings*/\n> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n> +\t\t\t\t *str, &len_domain, &pe_use);\n> +\t*(*str + len_domain) = '/';\n> +\tif (translate_sid_to_user == FALSE) {\n> +\t\tFREE_AND_NULL(*str);\n> +\t}\n\nIs this \"FREE_AND_NULL\" about clearing after you see an error? If\nso, shouldn't \"overwrite the byte after the domain part with a slash\"\nbe done only when you have no error, i.e., perhaps\n\n\ttranslate_sid_to_user = LookupAccountSidA(...);\n\tif (!translate_sid_to_user)\n\t\tFREE_AND_NULL(*str);\n\telse\n \t(*str)[len_domain] = '/';\n\nor something along the line?\n\n> +\treturn translate_sid_to_user;\n> +}\n"},{"id":"486209","messageId":"20240102191514.2583-2-soekkle@freenet.de","threadId":"60669","inReplyTo":"20240102191514.2583-1-soekkle@freenet.de","subject":"[PATCH V4 1/1] Replace SID with domain/username","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-01-02T19:15:14Z","receivedAt":"2024-01-02T19:20:37Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Replace SID with domain/username in error message, if owner of repository\nand user are not equal on windows systems. Each user should have a unique\nSID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).\nThis means that domain/username is not a loss of information. If the translation\nfails the message contains the SID as string.\n\nOld Prompted error message:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n\t'S-1-5-21-571067702-4104414259-3379520149-500'\nbut the current user is:\n\t'S-1-5-21-571067702-4104414259-3379520149-1001'\nTo add an exception for this directory, call:\n\n\tgit config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nNew prompted error massage:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n\t'DESKTOP-L78JVA6/Administrator'\nbut the current user is:\n\t'DESKTOP-L78JVA6/test'\nTo add an exception for this directory, call:\n\n\tgit config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nSigned-off-by: Sören Krecker \n---\n compat/mingw.c | 34 ++++++++++++++++++++++++++++++----\n 1 file changed, 30 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 42053c1f65..bfd9573a29 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2684,6 +2684,27 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n+{\n+\tSID_NAME_USE pe_use;\n+\tDWORD len_user = 0, len_domain = 0;\n+\tBOOL translate_sid_to_user;\n+\n+\t/* returns only FALSE, because the string pointers are NULL*/\n+\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n+\t\t\t &pe_use); \n+\t/*Alloc needed space of the strings*/\n+\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n+\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n+\t\t\t\t *str, &len_domain, &pe_use);\n+\tif (translate_sid_to_user == FALSE) {\n+\t\tFREE_AND_NULL(*str);\n+\t}\n+\telse\n+\t\t(*str)[len_domain] = '/';\n+\treturn translate_sid_to_user;\n+}\n+\n static int acls_supported(const char *path)\n {\n \tsize_t offset = offset_1st_component(path);\n@@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t} else if (report) {\n \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n \n-\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n+\t\t\tif (user_sid_to_user_name(sid, &str1))\n+\t\t\t\tto_free1 = str1;\n+\t\t\telse if (ConvertSidToStringSidA(sid, &str1))\n \t\t\t\tto_free1 = str1;\n \t\t\telse\n \t\t\t\tstr1 = \"(inconvertible)\";\n@@ -2776,7 +2799,10 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\t\tstr2 = \"(none)\";\n \t\t\telse if (!IsValidSid(current_user_sid))\n \t\t\t\tstr2 = \"(invalid)\";\n-\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n+\t\t\telse if (user_sid_to_user_name(current_user_sid, &str2))\n+\t\t\t\tto_free2 = str2;\n+\t\t\telse if (ConvertSidToStringSidA(current_user_sid,\n+\t\t\t\t\t\t\t&str2))\n \t\t\t\tto_free2 = str2;\n \t\t\telse\n \t\t\t\tstr2 = \"(inconvertible)\";\n@@ -2784,8 +2810,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\t\t \"'%s' is owned by:\\n\"\n \t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n \t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n-\t\t\tLocalFree(to_free1);\n-\t\t\tLocalFree(to_free2);\n+\t\t\tfree(to_free1);\n+\t\t\tfree(to_free2);\n \t\t}\n \t}\n \n-- \n2.39.2\n\n"},{"id":"486210","messageId":"20240102191514.2583-1-soekkle@freenet.de","threadId":"60669","inReplyTo":"xmqqsf3feilq.fsf@gitster.g","subject":"[PATCH V4 0/1] Replace SID with domain/username on Windows","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-01-02T19:15:13Z","receivedAt":"2024-01-02T19:20:47Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Hi everone,\n\nI improve the commit message with information from Microsoft, fixes a memmory access error and the message in case of an error.\n\nVielen dank für die Hinweise von Junio C Hamano.\n\nSören Krecker (1):\n Replace SID with domain/username\n\n compat/mingw.c | 34 ++++++++++++++++++++++++++++++----\n 1 file changed, 30 insertions(+), 4 deletions(-)\n\n\nbase-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n-- \n2.39.2\n\n"},{"id":"486226","messageId":"xmqqa5pnckm4.fsf@gitster.g","threadId":"60669","inReplyTo":"20240102191514.2583-2-soekkle@freenet.de","subject":"Re: [PATCH V4 1/1] Replace SID with domain/username","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-03T00:43:15Z","receivedAt":"2024-01-03T00:43:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> Replace SID with domain/username in error message, if owner of repository\n> and user are not equal on windows systems. Each user should have a unique\n> SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).\n\nThat paragraph your URL refers to does say that a SID that is used\nfor an account will never be reused to identify a different account.\nBut I am not sure if it means a user will never be assigned more\nthan one SID (in other words, the reverse is not necessarily true).\n\nThe paragraph also mentions that a SID can identify a non-user\nentity like a computer account (as opposed to \"a user account\")---I\ndo not know what its implications are in the context of this patch,\nthough.\n\n> This means that domain/username is not a loss of information.\n\nThis statement does not (grammatically) make sense, but more\nimportantly, loss of information may not be a bad thing in this\ncase. If more than one SIDs are given to a user account and\nprocesses working for that account, these different SIDs may be\ntranslated, by using LookupAccountSidA(), to the same string for a\nsingle user@domain, and it would be an operation that loses\ninformation in that sense.\n\nBut if what we *care* about is user@domain between the current\nprocess and the owner of the directory in question being the same\n(or not), then such a loss of information is a *good* thing. \n\nSo I dunno. Arguing what we care about (is that exact SID equality\nbetween the \"owner of the directory\" and the \"user, which the\ncurrent process is working on behalf of\", or do we care about the\nequality of the \"accounts\"?) may be a better way to justify this\nchange, if you ask me.\n\n> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n> +{\n> +\tSID_NAME_USE pe_use;\n> +\tDWORD len_user = 0, len_domain = 0;\n> +\tBOOL translate_sid_to_user;\n> +\n> +\t/* returns only FALSE, because the string pointers are NULL*/\n> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n> +\t\t\t &pe_use); \n> +\t/*Alloc needed space of the strings*/\n> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n> +\t\t\t\t *str, &len_domain, &pe_use);\n> +\tif (translate_sid_to_user == FALSE) {\n> +\t\tFREE_AND_NULL(*str);\n> +\t}\n\nStyle: do not enclose a single-statement block inside {}.\n\n> +\telse\n> +\t\t(*str)[len_domain] = '/';\n> +\treturn translate_sid_to_user;\n> +}\n\n> @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n> \t\t} else if (report) {\n> \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n> \n> -\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n> +\t\t\tif (user_sid_to_user_name(sid, &str1))\n> +\t\t\t\tto_free1 = str1;\n> +\t\t\telse if (ConvertSidToStringSidA(sid, &str1))\n> \t\t\t\tto_free1 = str1;\n\nDo these two helper functions return pointers pointing into the same\nkind of memory that you can free with the same function? That is ...\n\n> ...\n> \t\t\t\t \"'%s' is owned by:\\n\"\n> \t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n> \t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n> -\t\t\tLocalFree(to_free1);\n> -\t\t\tLocalFree(to_free2);\n> +\t\t\tfree(to_free1);\n> +\t\t\tfree(to_free2);\n\n... the original code seems to say that the piece of memory we\nobtain from ConvertSidToStringSidA() must not be freed by calling\nfree() but use something special called LocalFree(). I am assuing\nthat your user_sid_to_user_name() returns a regular piece of memory\nthat can be freed by calling regular free()? Do we need to keep\ntrack of where we got the memory from and use different function to\nfree each variable, or something (again I do not do Windows so I'll\ndefer all of these to Dscho, who is CC'ed this time).\n\nThanks and a happy new year.\n\n> \t\t}\n> \t}\n\n"},{"id":"486244","messageId":"DB9P250MB0692C8B4D93ED92FEE680AA9A560A@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM","threadId":"60669","inReplyTo":"xmqqa5pnckm4.fsf@gitster.g","subject":"Re: [PATCH V4 1/1] Replace SID with domain/username","fromName":"Matthias Aßhauer","fromEmail":"mha1993@live.de","sentAt":"2024-01-03T08:21:16Z","receivedAt":"2024-01-03T08:21:20Z","isPatch":true,"sender":{"key":"mha1993@live.de","avatar":"https://avatars.githubusercontent.com/u/6178234?v=4"},"body":"\n\nOn Tue, 2 Jan 2024, Junio C Hamano wrote:\n\n> Sören Krecker writes:\n>\n>> Replace SID with domain/username in error message, if owner of repository\n>> and user are not equal on windows systems. Each user should have a unique\n>> SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).\n>\n> That paragraph your URL refers to does say that a SID that is used\n> for an account will never be reused to identify a different account.\n> But I am not sure if it means a user will never be assigned more\n> than one SID (in other words, the reverse is not necessarily true).\n\nTo my knowledge a user account will never have multiple active SIDs, but\nthe documentation of LookupAccountSidA [1] explicitly mentions that it \ndoes look up historic SIDs.\n\n> In addition to looking up SIDs for local accounts, local domain \n> accounts, and explicitly trusted domain accounts, LookupAccountSid can \n> look up SIDs for any account in any domain in the forest, including SIDs \n> that appear only in the SIDhistory field of an account in the forest. \n> The SIDhistory field stores former SIDs of an account that has been\n> moved from another domain.\n\n[1] https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-lookupaccountsida#remarks\n\n>\n> The paragraph also mentions that a SID can identify a non-user\n> entity like a computer account (as opposed to \"a user account\")---I\n> do not know what its implications are in the context of this patch,\n> though.\n>\n>> This means that domain/username is not a loss of information.\n>\n> This statement does not (grammatically) make sense, but more\n> importantly, loss of information may not be a bad thing in this\n> case. If more than one SIDs are given to a user account and\n> processes working for that account, these different SIDs may be\n> translated, by using LookupAccountSidA(), to the same string for a\n> single user@domain, and it would be an operation that loses\n> information in that sense.\n>\n> But if what we *care* about is user@domain between the current\n> process and the owner of the directory in question being the same\n> (or not), then such a loss of information is a *good* thing.\n\nThis patch only changes the output of our error message, though.\nIt does not change what ownership information we actually compare.\nSo if we had a hypothetical user Bob that was part of the domain \nexample.com (SID S-1-5-21-100000001-1000000001-10000001-1001) and\nhad been moved over from the example.org domain (old SID S-1-5-21-\n2000000002-2000000002-20000002-2002) and we would detect a repository \nowned by bobs old SID, we would now lookup the old SID, find it \nattached to a user named example.com\\Bob, look up Bobs current SID, find \nit belongs to a user named example.com\\Bob and print a confusing error \nmessage.\n\n> So I dunno. Arguing what we care about (is that exact SID equality\n> between the \"owner of the directory\" and the \"user, which the\n> current process is working on behalf of\", or do we care about the\n> equality of the \"accounts\"?) may be a better way to justify this\n> change, if you ask me.\n>\n>> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n>> +{\n>> +\tSID_NAME_USE pe_use;\n>> +\tDWORD len_user = 0, len_domain = 0;\n>> +\tBOOL translate_sid_to_user;\n>> +\n>> +\t/* returns only FALSE, because the string pointers are NULL*/\n>> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n>> +\t\t\t &pe_use);\n>> +\t/*Alloc needed space of the strings*/\n>> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);\n>> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n>> +\t\t\t\t *str, &len_domain, &pe_use);\n>> +\tif (translate_sid_to_user == FALSE) {\n>> +\t\tFREE_AND_NULL(*str);\n>> +\t}\n>\n> Style: do not enclose a single-statement block inside {}.\n>\n>> +\telse\n>> +\t\t(*str)[len_domain] = '/';\n>> +\treturn translate_sid_to_user;\n>> +}\n>\n>> @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n>> \t\t} else if (report) {\n>> \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n>>\n>> -\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n>> +\t\t\tif (user_sid_to_user_name(sid, &str1))\n>> +\t\t\t\tto_free1 = str1;\n>> +\t\t\telse if (ConvertSidToStringSidA(sid, &str1))\n>> \t\t\t\tto_free1 = str1;\n>\n> Do these two helper functions return pointers pointing into the same\n> kind of memory that you can free with the same function? That is ...\n>\n>> ...\n>> \t\t\t\t \"'%s' is owned by:\\n\"\n>> \t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n>> \t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n>> -\t\t\tLocalFree(to_free1);\n>> -\t\t\tLocalFree(to_free2);\n>> +\t\t\tfree(to_free1);\n>> +\t\t\tfree(to_free2);\n>\n> ... the original code seems to say that the piece of memory we\n> obtain from ConvertSidToStringSidA() must not be freed by calling\n> free() but use something special called LocalFree(). I am assuing\n> that your user_sid_to_user_name() returns a regular piece of memory\n> that can be freed by calling regular free()? Do we need to keep\n> track of where we got the memory from and use different function to\n> free each variable, or something (again I do not do Windows so I'll\n> defer all of these to Dscho, who is CC'ed this time).\n>\n> Thanks and a happy new year.\n>\n>> \t\t}\n>> \t}\n>\n>\n"},{"id":"486278","messageId":"xmqqil4a83b1.fsf@gitster.g","threadId":"60669","inReplyTo":"DB9P250MB0692C8B4D93ED92FEE680AA9A560A@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM","subject":"Re: [PATCH V4 1/1] Replace SID with domain/username","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-03T22:22:58Z","receivedAt":"2024-01-03T22:23:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthias Aßhauer writes:\n\n> This patch only changes the output of our error message, though.\n> It does not change what ownership information we actually compare.\n> So if we had a hypothetical user Bob that was part of the domain\n> example.com (SID S-1-5-21-100000001-1000000001-10000001-1001) and\n> had been moved over from the example.org domain (old SID S-1-5-21-\n> 2000000002-2000000002-20000002-2002) and we would detect a repository\n> owned by bobs old SID, we would now lookup the old SID, find it\n> attached to a user named example.com\\Bob, look up Bobs current SID,\n> find it belongs to a user named example.com\\Bob and print a confusing\n> error message.\n\nYup, that is exactly the kind of breakage I was worried about.\n\nPerhaps we should do something along the lines of ...\n\n - The erroring out should be done purely by SID comparison, as that\n is what we have been doing to protect the users.\n\n - When creating a message, use LookupAccountSidA() to come up with\n a pair of domain\\user strings for the directory and the process\n to be used in the error message:\n\n - If they are different (which is expected to be the normal\n case), we just use the pair of strings.\n\n - If they are the same, show old and new SID in stringified form\n (hopefully different SIDs would strigify to different\n strings?), and optionally we give the domain\\user string next\n to it.\n\n... then? Then we would emit an error message (in the best case)\n\n 'directory' is owned by:\n 'bob@example.org'\n but the current user is:\n 'charlie@example.com'\n\nand in a bad case we would instead see something like:\n\n 'directory' is owned by:\n SID S-1-5-21-100000001-1000000001-10000001-1001 ('bob@example.org')\n but the current user is:\n SID S-1-5-21-200000002-2000000002-20000002-2002 ('bob@example.org')\n\nwhich may still be serviceable. I dunno.\n\n"},{"id":"486309","messageId":"20240104192202.2124-1-soekkle@freenet.de","threadId":"60669","inReplyTo":"DB9P250MB0692C8B4D93ED92FEE680AA9A560A@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM","subject":"[PATCH v5 0/1] Replace SID with domain/username on Windows","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-01-04T19:22:01Z","receivedAt":"2024-01-04T19:22:20Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Hi everyone,\n\nI change the error message. I Hope that it is now better for every one.\n\nThanks\n\nSören Krecker (1):\n Adds domain/username to error message\n\n compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 51 insertions(+), 13 deletions(-)\n\n\nbase-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n-- \n2.39.2\n\n"},{"id":"486308","messageId":"20240104192202.2124-2-soekkle@freenet.de","threadId":"60669","inReplyTo":"20240104192202.2124-1-soekkle@freenet.de","subject":"[PATCH v5 1/1] Adds domain/username to error message","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-01-04T19:22:02Z","receivedAt":"2024-01-04T19:22:21Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Adds domain/username in error message, if owner sid of repository and\nuser sid are not equal on windows systems.\n\nOld Prompted error message:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n\t'S-1-5-21-571067702-4104414259-3379520149-500'\nbut the current user is:\n\t'S-1-5-21-571067702-4104414259-3379520149-1001'\nTo add an exception for this directory, call:\n\n\tgit config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nNew prompted error massage:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n 'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)\nbut the current user is:\n 'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)\nTo add an exception for this directory, call:\n\n git config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nSigned-off-by: Sören Krecker \n---\n compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 51 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 42053c1f65..6240387205 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n+{\n+\tSID_NAME_USE pe_use;\n+\tDWORD len_user = 0, len_domain = 0;\n+\tBOOL translate_sid_to_user;\n+\n+\t/* returns only FALSE, because the string pointers are NULL*/\n+\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n+\t\t\t &pe_use); \n+\t/*Alloc needed space of the strings*/\n+\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n+\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n+\t\t\t\t *str, &len_domain, &pe_use);\n+\tif (translate_sid_to_user == FALSE)\n+\t\tFREE_AND_NULL(*str);\n+\telse\n+\t\t(*str)[len_domain] = '/';\n+\treturn translate_sid_to_user;\n+}\n+\n static int acls_supported(const char *path)\n {\n \tsize_t offset = offset_1st_component(path);\n@@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\tstrbuf_addf(report, \"'%s' is on a file system that does \"\n \t\t\t\t \"not record ownership\\n\", path);\n \t\t} else if (report) {\n-\t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n+\t\t\tLPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;\n \n-\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n+\t\t\tif (user_sid_to_user_name(sid, &str1))\n \t\t\t\tto_free1 = str1;\n \t\t\telse\n \t\t\t\tstr1 = \"(inconvertible)\";\n-\n-\t\t\tif (!current_user_sid)\n-\t\t\t\tstr2 = \"(none)\";\n-\t\t\telse if (!IsValidSid(current_user_sid))\n-\t\t\t\tstr2 = \"(invalid)\";\n-\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n-\t\t\t\tto_free2 = str2;\n+\t\t\tif (ConvertSidToStringSidA(sid, &str2))\n+\t\t\t\tto_local_free2 = str2;\n \t\t\telse\n \t\t\t\tstr2 = \"(inconvertible)\";\n+\n+\t\t\tif (!current_user_sid) {\n+\t\t\t\tstr3 = \"(none)\";\n+\t\t\t\tstr4 = \"(none)\";\n+\t\t\t}\n+\t\t\telse if (!IsValidSid(current_user_sid)) {\n+\t\t\t\tstr3 = \"(invalid)\";\n+\t\t\t\tstr4 = \"(invalid)\";\n+\t\t\t} else {\n+\t\t\t\tif (user_sid_to_user_name(current_user_sid,\n+\t\t\t\t\t\t\t &str3))\n+\t\t\t\t\tto_free3 = str3;\n+\t\t\t\telse\n+\t\t\t\t\tstr3 = \"(inconvertible)\";\n+\t\t\t\tif (ConvertSidToStringSidA(current_user_sid,\n+\t\t\t\t\t\t\t &str4))\n+\t\t\t\t\tto_local_free4 = str4;\n+\t\t\t\telse\n+\t\t\t\t\tstr4 = \"(inconvertible)\";\n+\t\t\t}\n \t\t\tstrbuf_addf(report,\n \t\t\t\t \"'%s' is owned by:\\n\"\n-\t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n-\t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n-\t\t\tLocalFree(to_free1);\n-\t\t\tLocalFree(to_free2);\n+\t\t\t\t \"\\t'%s' (%s)\\nbut the current user is:\\n\"\n+\t\t\t\t \"\\t'%s' (%s)\\n\",\n+\t\t\t\t path, str1, str2, str3, str4);\n+\t\t\tfree(to_free1);\n+\t\t\tLocalFree(to_local_free2);\n+\t\t\tfree(to_free3);\n+\t\t\tLocalFree(to_local_free4);\n \t\t}\n \t}\n \n-- \n2.39.2\n\n"},{"id":"486311","messageId":"xmqqbka07te6.fsf@gitster.g","threadId":"60669","inReplyTo":"20240104192202.2124-2-soekkle@freenet.de","subject":"Re: [PATCH v5 1/1] Adds domain/username to error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-04T20:09:21Z","receivedAt":"2024-01-04T20:09:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> Subject: Re: [PATCH v5 1/1] Adds domain/username to error message\n\nLooking at past commits that worked on the area this patch touches,\nnamely, 7c83470e (mingw: be more informative when ownership check\nfails on FAT32, 2022-08-08) and e883e04b (mingw: provide details\nabout unsafe directories' ownership, 2022-08-08), I would retitle\nthe commit perhaps like so:\n\n Subject: [PATCH v5] mingw: give more details about unsafe directory's ownership\n\nif I were doing this patch.\n\n> Adds domain/username in error message, if owner sid of repository and\n\n\"Adds\" -> \"Add\".\n\n> user sid are not equal on windows systems.\n>\n> Old Prompted error message:\n> '''\n> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n> 'C:/Users/test/source/repos/git' is owned by:\n> \t'S-1-5-21-571067702-4104414259-3379520149-500'\n> but the current user is:\n> \t'S-1-5-21-571067702-4104414259-3379520149-1001'\n> To add an exception for this directory, call:\n>\n> \tgit config --global --add safe.directory C:/Users/test/source/repos/git\n> '''\n>\n> New prompted error massage:\n\n\"massage\" -> \"message\".\n\nI probably would drop two \"prompted\" from the above, too, if I were\ndoing this patch.\n\nThanks for working on making this error message more readable. I'll\nqueue it when I see an Ack from Dscho.\n\n\n\n\n\n> '''\n> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n> 'C:/Users/test/source/repos/git' is owned by:\n> 'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)\n> but the current user is:\n> 'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)\n> To add an exception for this directory, call:\n>\n> git config --global --add safe.directory C:/Users/test/source/repos/git\n> '''\n>\n> Signed-off-by: Sören Krecker \n> ---\n> compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------\n> 1 file changed, 51 insertions(+), 13 deletions(-)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 42053c1f65..6240387205 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)\n> \treturn result;\n> }\n> \n> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n> +{\n> +\tSID_NAME_USE pe_use;\n> +\tDWORD len_user = 0, len_domain = 0;\n> +\tBOOL translate_sid_to_user;\n> +\n> +\t/* returns only FALSE, because the string pointers are NULL*/\n> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n> +\t\t\t &pe_use); \n> +\t/*Alloc needed space of the strings*/\n> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n> +\t\t\t\t *str, &len_domain, &pe_use);\n> +\tif (translate_sid_to_user == FALSE)\n> +\t\tFREE_AND_NULL(*str);\n> +\telse\n> +\t\t(*str)[len_domain] = '/';\n> +\treturn translate_sid_to_user;\n> +}\n> +\n> static int acls_supported(const char *path)\n> {\n> \tsize_t offset = offset_1st_component(path);\n> @@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n> \t\t\tstrbuf_addf(report, \"'%s' is on a file system that does \"\n> \t\t\t\t \"not record ownership\\n\", path);\n> \t\t} else if (report) {\n> -\t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n> +\t\t\tLPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;\n> \n> -\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n> +\t\t\tif (user_sid_to_user_name(sid, &str1))\n> \t\t\t\tto_free1 = str1;\n> \t\t\telse\n> \t\t\t\tstr1 = \"(inconvertible)\";\n> -\n> -\t\t\tif (!current_user_sid)\n> -\t\t\t\tstr2 = \"(none)\";\n> -\t\t\telse if (!IsValidSid(current_user_sid))\n> -\t\t\t\tstr2 = \"(invalid)\";\n> -\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n> -\t\t\t\tto_free2 = str2;\n> +\t\t\tif (ConvertSidToStringSidA(sid, &str2))\n> +\t\t\t\tto_local_free2 = str2;\n> \t\t\telse\n> \t\t\t\tstr2 = \"(inconvertible)\";\n> +\n> +\t\t\tif (!current_user_sid) {\n> +\t\t\t\tstr3 = \"(none)\";\n> +\t\t\t\tstr4 = \"(none)\";\n> +\t\t\t}\n> +\t\t\telse if (!IsValidSid(current_user_sid)) {\n> +\t\t\t\tstr3 = \"(invalid)\";\n> +\t\t\t\tstr4 = \"(invalid)\";\n> +\t\t\t} else {\n> +\t\t\t\tif (user_sid_to_user_name(current_user_sid,\n> +\t\t\t\t\t\t\t &str3))\n> +\t\t\t\t\tto_free3 = str3;\n> +\t\t\t\telse\n> +\t\t\t\t\tstr3 = \"(inconvertible)\";\n> +\t\t\t\tif (ConvertSidToStringSidA(current_user_sid,\n> +\t\t\t\t\t\t\t &str4))\n> +\t\t\t\t\tto_local_free4 = str4;\n> +\t\t\t\telse\n> +\t\t\t\t\tstr4 = \"(inconvertible)\";\n> +\t\t\t}\n> \t\t\tstrbuf_addf(report,\n> \t\t\t\t \"'%s' is owned by:\\n\"\n> -\t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n> -\t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n> -\t\t\tLocalFree(to_free1);\n> -\t\t\tLocalFree(to_free2);\n> +\t\t\t\t \"\\t'%s' (%s)\\nbut the current user is:\\n\"\n> +\t\t\t\t \"\\t'%s' (%s)\\n\",\n> +\t\t\t\t path, str1, str2, str3, str4);\n> +\t\t\tfree(to_free1);\n> +\t\t\tLocalFree(to_local_free2);\n> +\t\t\tfree(to_free3);\n> +\t\t\tLocalFree(to_local_free4);\n> \t\t}\n> \t}\n"},{"id":"486360","messageId":"20240106112917.1870-1-soekkle@freenet.de","threadId":"60669","inReplyTo":"xmqqbka07te6.fsf@gitster.g","subject":"[PATCH v6 0/1] mingw: give more details about unsafe directory's ownership","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-01-06T11:29:16Z","receivedAt":"2024-01-06T11:34:44Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"So I change the commit message and the title of this commit.\n\nThanks for your feedback.\n\nSören Krecker (1):\n mingw: give more details about unsafe directory's ownership\n\n compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 51 insertions(+), 13 deletions(-)\n\n\nbase-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n-- \n2.39.2\n\n"},{"id":"486361","messageId":"20240106112917.1870-2-soekkle@freenet.de","threadId":"60669","inReplyTo":"20240106112917.1870-1-soekkle@freenet.de","subject":"[PATCH v6 1/1] mingw: give more details about unsafe directory's ownership","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-01-06T11:29:17Z","receivedAt":"2024-01-06T11:35:18Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Add domain/username in error message, if owner sid of repository and\nuser sid are not equal on windows systems.\n\nOld error message:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n\t'S-1-5-21-571067702-4104414259-3379520149-500'\nbut the current user is:\n\t'S-1-5-21-571067702-4104414259-3379520149-1001'\nTo add an exception for this directory, call:\n\n\tgit config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nNew error message:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n 'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)\nbut the current user is:\n 'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)\nTo add an exception for this directory, call:\n\n git config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nSigned-off-by: Sören Krecker \n---\n compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 51 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 42053c1f65..6240387205 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n+{\n+\tSID_NAME_USE pe_use;\n+\tDWORD len_user = 0, len_domain = 0;\n+\tBOOL translate_sid_to_user;\n+\n+\t/* returns only FALSE, because the string pointers are NULL*/\n+\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n+\t\t\t &pe_use); \n+\t/*Alloc needed space of the strings*/\n+\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n+\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n+\t\t\t\t *str, &len_domain, &pe_use);\n+\tif (translate_sid_to_user == FALSE)\n+\t\tFREE_AND_NULL(*str);\n+\telse\n+\t\t(*str)[len_domain] = '/';\n+\treturn translate_sid_to_user;\n+}\n+\n static int acls_supported(const char *path)\n {\n \tsize_t offset = offset_1st_component(path);\n@@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\tstrbuf_addf(report, \"'%s' is on a file system that does \"\n \t\t\t\t \"not record ownership\\n\", path);\n \t\t} else if (report) {\n-\t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n+\t\t\tLPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;\n \n-\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n+\t\t\tif (user_sid_to_user_name(sid, &str1))\n \t\t\t\tto_free1 = str1;\n \t\t\telse\n \t\t\t\tstr1 = \"(inconvertible)\";\n-\n-\t\t\tif (!current_user_sid)\n-\t\t\t\tstr2 = \"(none)\";\n-\t\t\telse if (!IsValidSid(current_user_sid))\n-\t\t\t\tstr2 = \"(invalid)\";\n-\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n-\t\t\t\tto_free2 = str2;\n+\t\t\tif (ConvertSidToStringSidA(sid, &str2))\n+\t\t\t\tto_local_free2 = str2;\n \t\t\telse\n \t\t\t\tstr2 = \"(inconvertible)\";\n+\n+\t\t\tif (!current_user_sid) {\n+\t\t\t\tstr3 = \"(none)\";\n+\t\t\t\tstr4 = \"(none)\";\n+\t\t\t}\n+\t\t\telse if (!IsValidSid(current_user_sid)) {\n+\t\t\t\tstr3 = \"(invalid)\";\n+\t\t\t\tstr4 = \"(invalid)\";\n+\t\t\t} else {\n+\t\t\t\tif (user_sid_to_user_name(current_user_sid,\n+\t\t\t\t\t\t\t &str3))\n+\t\t\t\t\tto_free3 = str3;\n+\t\t\t\telse\n+\t\t\t\t\tstr3 = \"(inconvertible)\";\n+\t\t\t\tif (ConvertSidToStringSidA(current_user_sid,\n+\t\t\t\t\t\t\t &str4))\n+\t\t\t\t\tto_local_free4 = str4;\n+\t\t\t\telse\n+\t\t\t\t\tstr4 = \"(inconvertible)\";\n+\t\t\t}\n \t\t\tstrbuf_addf(report,\n \t\t\t\t \"'%s' is owned by:\\n\"\n-\t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n-\t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n-\t\t\tLocalFree(to_free1);\n-\t\t\tLocalFree(to_free2);\n+\t\t\t\t \"\\t'%s' (%s)\\nbut the current user is:\\n\"\n+\t\t\t\t \"\\t'%s' (%s)\\n\",\n+\t\t\t\t path, str1, str2, str3, str4);\n+\t\t\tfree(to_free1);\n+\t\t\tLocalFree(to_local_free2);\n+\t\t\tfree(to_free3);\n+\t\t\tLocalFree(to_local_free4);\n \t\t}\n \t}\n \n-- \n2.39.2\n\n"},{"id":"486379","messageId":"de9cf40a-1ad6-45fb-8b70-8b0c71a3bfbb@kdbg.org","threadId":"60669","inReplyTo":"20240106112917.1870-2-soekkle@freenet.de","subject":"Re: [PATCH v6 1/1] mingw: give more details about unsafe directory's ownership","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-01-07T20:02:09Z","receivedAt":"2024-01-07T20:02:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.01.24 um 12:29 schrieb Sören Krecker:\n> Add domain/username in error message, if owner sid of repository and\n> user sid are not equal on windows systems.\n> \n> Old error message:\n> '''\n> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n> 'C:/Users/test/source/repos/git' is owned by:\n> \t'S-1-5-21-571067702-4104414259-3379520149-500'\n> but the current user is:\n> \t'S-1-5-21-571067702-4104414259-3379520149-1001'\n> To add an exception for this directory, call:\n> \n> \tgit config --global --add safe.directory C:/Users/test/source/repos/git\n> '''\n> \n> New error message:\n> '''\n> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n> 'C:/Users/test/source/repos/git' is owned by:\n> 'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)\n> but the current user is:\n> 'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)\n> To add an exception for this directory, call:\n> \n> git config --global --add safe.directory C:/Users/test/source/repos/git\n> '''\n\nI am not a fan of putting everything and the kitchen sink inside quotes.\nIn particular, the single-quotes around the user names are unnecessary,\nIMO. Would you mind dropping them? (I do not mean to remove the quotes\naround the path names because you do not touch this part of the code.)\n\n> \n> Signed-off-by: Sören Krecker \n> ---\n> compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------\n> 1 file changed, 51 insertions(+), 13 deletions(-)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 42053c1f65..6240387205 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)\n> \treturn result;\n> }\n> \n> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n> +{\n> +\tSID_NAME_USE pe_use;\n> +\tDWORD len_user = 0, len_domain = 0;\n> +\tBOOL translate_sid_to_user;\n> +\n> +\t/* returns only FALSE, because the string pointers are NULL*/\n> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n> +\t\t\t &pe_use); \n> +\t/*Alloc needed space of the strings*/\n\nThis comment line doesn't follow our style. Please either fix that (add\nblanks after /* and before */ or (my preference) remove it altogether;\nthe code is clear without it.\n\n> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,\n> +\t\t\t\t *str, &len_domain, &pe_use);\n> +\tif (translate_sid_to_user == FALSE)\n\nWe prefer to write this condition as\n\n\tif (!translate_sid_to_user)\n\n> +\t\tFREE_AND_NULL(*str);\n> +\telse\n> +\t\t(*str)[len_domain] = '/';\n> +\treturn translate_sid_to_user;\n> +}\n> +\n> static int acls_supported(const char *path)\n> {\n> \tsize_t offset = offset_1st_component(path);\n> @@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n> \t\t\tstrbuf_addf(report, \"'%s' is on a file system that does \"\n> \t\t\t\t \"not record ownership\\n\", path);\n> \t\t} else if (report) {\n> -\t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n> +\t\t\tLPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;\n\nThis line grew a bit long now. Maybe break it to have lines not exceed\n80 characters? While you do so, please insert blanks around = signs.\n\n> \n> -\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n> +\t\t\tif (user_sid_to_user_name(sid, &str1))\n> \t\t\t\tto_free1 = str1;\n> \t\t\telse\n> \t\t\t\tstr1 = \"(inconvertible)\";\n> -\n> -\t\t\tif (!current_user_sid)\n> -\t\t\t\tstr2 = \"(none)\";\n> -\t\t\telse if (!IsValidSid(current_user_sid))\n> -\t\t\t\tstr2 = \"(invalid)\";\n> -\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n> -\t\t\t\tto_free2 = str2;\n> +\t\t\tif (ConvertSidToStringSidA(sid, &str2))\n> +\t\t\t\tto_local_free2 = str2;\n> \t\t\telse\n> \t\t\t\tstr2 = \"(inconvertible)\";\n> +\n> +\t\t\tif (!current_user_sid) {\n> +\t\t\t\tstr3 = \"(none)\";\n> +\t\t\t\tstr4 = \"(none)\";\n> +\t\t\t}\n> +\t\t\telse if (!IsValidSid(current_user_sid)) {\n> +\t\t\t\tstr3 = \"(invalid)\";\n> +\t\t\t\tstr4 = \"(invalid)\";\n> +\t\t\t} else {\n> +\t\t\t\tif (user_sid_to_user_name(current_user_sid,\n> +\t\t\t\t\t\t\t &str3))\n> +\t\t\t\t\tto_free3 = str3;\n> +\t\t\t\telse\n> +\t\t\t\t\tstr3 = \"(inconvertible)\";\n> +\t\t\t\tif (ConvertSidToStringSidA(current_user_sid,\n> +\t\t\t\t\t\t\t &str4))\n> +\t\t\t\t\tto_local_free4 = str4;\n> +\t\t\t\telse\n> +\t\t\t\t\tstr4 = \"(inconvertible)\";\n> +\t\t\t}\n> \t\t\tstrbuf_addf(report,\n> \t\t\t\t \"'%s' is owned by:\\n\"\n> -\t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n> -\t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n> -\t\t\tLocalFree(to_free1);\n> -\t\t\tLocalFree(to_free2);\n> +\t\t\t\t \"\\t'%s' (%s)\\nbut the current user is:\\n\"\n> +\t\t\t\t \"\\t'%s' (%s)\\n\",\n> +\t\t\t\t path, str1, str2, str3, str4);\n> +\t\t\tfree(to_free1);\n> +\t\t\tLocalFree(to_local_free2);\n> +\t\t\tfree(to_free3);\n> +\t\t\tLocalFree(to_local_free4);\n> \t\t}\n> \t}\n> \n\nAside from this, the patch works well for me. It is a real usability\nimprovement. Thank you for working on it.\n\n-- Hannes\n\n"},{"id":"486400","messageId":"20240108173837.20480-1-soekkle@freenet.de","threadId":"60669","inReplyTo":"de9cf40a-1ad6-45fb-8b70-8b0c71a3bfbb@kdbg.org","subject":"[PATCH v6 0/1] mingw: give more details about unsafe directory's","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-01-08T17:38:36Z","receivedAt":"2024-01-08T17:39:02Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"I have processed the points raised.\n\n\nSören Krecker (1):\n mingw: give more details about unsafe directory's ownership\n\n compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 57 insertions(+), 13 deletions(-)\n\n\nbase-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n-- \n2.39.2\n\n"},{"id":"486401","messageId":"20240108173837.20480-2-soekkle@freenet.de","threadId":"60669","inReplyTo":"20240108173837.20480-1-soekkle@freenet.de","subject":"[PATCH v7 1/1] mingw: give more details about unsafe directory's ownership","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2024-01-08T17:38:37Z","receivedAt":"2024-01-08T17:39:05Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Add domain/username in error message, if owner sid of repository and\nuser sid are not equal on windows systems.\n\nOld error message:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n\t'S-1-5-21-571067702-4104414259-3379520149-500'\nbut the current user is:\n\t'S-1-5-21-571067702-4104414259-3379520149-1001'\nTo add an exception for this directory, call:\n\n\tgit config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nNew error message:\n'''\nfatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'\n'C:/Users/test/source/repos/git' is owned by:\n DESKTOP-L78JVA6/Administrator (S-1-5-21-571067702-4104414259-3379520149-500)\nbut the current user is:\n DESKTOP-L78JVA6/test (S-1-5-21-571067702-4104414259-3379520149-1001)\nTo add an exception for this directory, call:\n\n git config --global --add safe.directory C:/Users/test/source/repos/git\n'''\n\nSigned-off-by: Sören Krecker \n---\n compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 57 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 42053c1f65..d85fae3747 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2684,6 +2684,30 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n+{\n+\tSID_NAME_USE pe_use;\n+\tDWORD len_user = 0, len_domain = 0;\n+\tBOOL translate_sid_to_user;\n+\n+\t/*\n+\t * returns only FALSE, because the string pointers are NULL\n+\t */\n+\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n+\t\t\t &pe_use); \n+\t/*\n+\t * Alloc needed space of the strings\n+\t */\n+\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n+\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid,\n+\t (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);\n+\tif (!translate_sid_to_user)\n+\t\tFREE_AND_NULL(*str);\n+\telse\n+\t\t(*str)[len_domain] = '/';\n+\treturn translate_sid_to_user;\n+}\n+\n static int acls_supported(const char *path)\n {\n \tsize_t offset = offset_1st_component(path);\n@@ -2765,27 +2789,47 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t\tstrbuf_addf(report, \"'%s' is on a file system that does \"\n \t\t\t\t \"not record ownership\\n\", path);\n \t\t} else if (report) {\n-\t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n+\t\t\tLPSTR str1, str2, str3, str4, to_free1 = NULL, \n+\t\t\t to_free3 = NULL, to_local_free2 = NULL,\n+\t\t\t to_local_free4 = NULL;\n \n-\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n+\t\t\tif (user_sid_to_user_name(sid, &str1))\n \t\t\t\tto_free1 = str1;\n \t\t\telse\n \t\t\t\tstr1 = \"(inconvertible)\";\n-\n-\t\t\tif (!current_user_sid)\n-\t\t\t\tstr2 = \"(none)\";\n-\t\t\telse if (!IsValidSid(current_user_sid))\n-\t\t\t\tstr2 = \"(invalid)\";\n-\t\t\telse if (ConvertSidToStringSidA(current_user_sid, &str2))\n-\t\t\t\tto_free2 = str2;\n+\t\t\tif (ConvertSidToStringSidA(sid, &str2))\n+\t\t\t\tto_local_free2 = str2;\n \t\t\telse\n \t\t\t\tstr2 = \"(inconvertible)\";\n+\n+\t\t\tif (!current_user_sid) {\n+\t\t\t\tstr3 = \"(none)\";\n+\t\t\t\tstr4 = \"(none)\";\n+\t\t\t}\n+\t\t\telse if (!IsValidSid(current_user_sid)) {\n+\t\t\t\tstr3 = \"(invalid)\";\n+\t\t\t\tstr4 = \"(invalid)\";\n+\t\t\t} else {\n+\t\t\t\tif (user_sid_to_user_name(current_user_sid,\n+\t\t\t\t\t\t\t &str3))\n+\t\t\t\t\tto_free3 = str3;\n+\t\t\t\telse\n+\t\t\t\t\tstr3 = \"(inconvertible)\";\n+\t\t\t\tif (ConvertSidToStringSidA(current_user_sid,\n+\t\t\t\t\t\t\t &str4))\n+\t\t\t\t\tto_local_free4 = str4;\n+\t\t\t\telse\n+\t\t\t\t\tstr4 = \"(inconvertible)\";\n+\t\t\t}\n \t\t\tstrbuf_addf(report,\n \t\t\t\t \"'%s' is owned by:\\n\"\n-\t\t\t\t \"\\t'%s'\\nbut the current user is:\\n\"\n-\t\t\t\t \"\\t'%s'\\n\", path, str1, str2);\n-\t\t\tLocalFree(to_free1);\n-\t\t\tLocalFree(to_free2);\n+\t\t\t\t \"\\t%s (%s)\\nbut the current user is:\\n\"\n+\t\t\t\t \"\\t%s (%s)\\n\",\n+\t\t\t\t path, str1, str2, str3, str4);\n+\t\t\tfree(to_free1);\n+\t\t\tLocalFree(to_local_free2);\n+\t\t\tfree(to_free3);\n+\t\t\tLocalFree(to_local_free4);\n \t\t}\n \t}\n \n-- \n2.39.2\n\n"},{"id":"486402","messageId":"c0604d0bb667e0ab872021eee2747e3e@manjaro.org","threadId":"60669","inReplyTo":"20240108173837.20480-1-soekkle@freenet.de","subject":"Re: [PATCH v6 0/1] mingw: give more details about unsafe directory's","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-01-08T17:51:32Z","receivedAt":"2024-01-08T17:51:36Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-01-08 18:38, Sören Krecker wrote:\n> I have processed the points raised.\n\nIf I may suggest, there's no need for a cover letter for a single patch. \n If you want to include some notes in the patch submission, which aren't \nsupposed to be part of the commit summary, you can do that in the patch \nitself.\n\n> Sören Krecker (1):\n> mingw: give more details about unsafe directory's ownership\n> \n> compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------\n> 1 file changed, 57 insertions(+), 13 deletions(-)\n> \n> \n> base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa\n"},{"id":"486408","messageId":"xmqqle8zk50r.fsf@gitster.g","threadId":"60669","inReplyTo":"20240108173837.20480-2-soekkle@freenet.de","subject":"Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-08T19:18:44Z","receivedAt":"2024-01-08T19:18:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sören Krecker writes:\n\n> Add domain/username in error message, if owner sid of repository and\n> user sid are not equal on windows systems.\n\nWill queue. \"git am --whitespace=fix\" noticed numerous whitespace\nbreakages in the patch, which has been corrected on the receiving\nend. Please make sure your future patches will not have such\nproblems.\n\nThanks.\n\n\n$ git am -s3c ./+sk-v7-mingw-ownership-report\nApplying: mingw: give more details about unsafe directory's ownership\n.git/rebase-apply/patch:23: trailing whitespace.\n\t\t\t &pe_use); \n.git/rebase-apply/patch:27: trailing whitespace.\n\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n.git/rebase-apply/patch:45: trailing whitespace.\n\t\t\tLPSTR str1, str2, str3, str4, to_free1 = NULL, \nwarning: 3 lines add whitespace errors.\n.git/rebase-apply/patch:23: trailing whitespace.\n\t\t\t &pe_use); \n.git/rebase-apply/patch:27: trailing whitespace.\n\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n.git/rebase-apply/patch:45: trailing whitespace.\n\t\t\tLPSTR str1, str2, str3, str4, to_free1 = NULL, \nwarning: 3 lines applied after fixing whitespace errors.\nUsing index info to reconstruct a base tree...\nM\tcompat/mingw.c\nFalling back to patching base and 3-way merge...\nAuto-merging compat/mingw.c\n"},{"id":"486464","messageId":"d1e1a543-ab9c-4b1b-9f1d-3728e791df2e@kdbg.org","threadId":"60669","inReplyTo":"20240108173837.20480-2-soekkle@freenet.de","subject":"Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-01-09T19:27:35Z","receivedAt":"2024-01-09T19:27:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.01.24 um 18:38 schrieb Sören Krecker:\n> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)\n> +{\n> +\tSID_NAME_USE pe_use;\n> +\tDWORD len_user = 0, len_domain = 0;\n> +\tBOOL translate_sid_to_user;\n> +\n> +\t/*\n> +\t * returns only FALSE, because the string pointers are NULL\n> +\t */\n> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n> +\t\t\t &pe_use); \n\nAt this point, the function fails, so len_user and len_domain contain\nthe required buffer size (including the trailing NUL).\n\n> +\t/*\n> +\t * Alloc needed space of the strings\n> +\t */\n> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid,\n> +\t (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);\n\nAt this point, if the function is successful, len_user and len_domain\ncontain the lengths of the names (without the trailing NUL).\n\n> +\tif (!translate_sid_to_user)\n> +\t\tFREE_AND_NULL(*str);\n> +\telse\n> +\t\t(*str)[len_domain] = '/';\n\nTherefore, this overwrites the NUL after the domain name and so\nconcatenates the two names. Good.\n\nI found this by dumping the values of the variables, because the\ndocumentation of LookupAccountSid is not clear about the values that the\nvariables receive in the success case.\n\n> +\treturn translate_sid_to_user;\n> +}\n> +\n\nThis patch looks good and works for me.\n\nAcked-by: Johannes Sixt \n\nThank you!\n\n-- Hannes\n\n"},{"id":"486470","messageId":"xmqq8r4ygtkd.fsf@gitster.g","threadId":"60669","inReplyTo":"d1e1a543-ab9c-4b1b-9f1d-3728e791df2e@kdbg.org","subject":"Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T20:06:42Z","receivedAt":"2024-01-09T20:06:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n>> +\t\t\t &pe_use); \n>\n> At this point, the function fails, so len_user and len_domain contain\n> the required buffer size (including the trailing NUL).\n\nSo (*str)[len_domain] would be the trailing NUL after the domain\npart in the next call? Or would that be (*str)[len_domain-1]? I am\npuzzled by off-by-one with your \"including the trailing NUL\" remark.\n\n>> +\t/*\n>> +\t * Alloc needed space of the strings\n>> +\t */\n>> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n\nThis obviously assumes for domain 'd' and user 'u', we want \"d/u\" and\nlen_domain must be 1+1 (including NUL) and len_user must be 1+1\n(including NUL). But then ...\n\n>> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid,\n>> +\t (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);\n\n... ((*str)+len_domain) is presumably the beginning of the user\npart, and (*str)+0) is where the domain part is to be stored.\n\nBecause len_domain includes the terminating NUL for the domain part,\n(*str)[len_domain-1] is that NUL, no? And that is what you want to\noverwrite to make the two strings into a single\none . So...\n\n> At this point, if the function is successful, len_user and len_domain\n> contain the lengths of the names (without the trailing NUL).\n>\n>> +\tif (!translate_sid_to_user)\n>> +\t\tFREE_AND_NULL(*str);\n>> +\telse\n>> +\t\t(*str)[len_domain] = '/';\n\n... this offset looks fishy to me. Am I off-by-one?\n\n> Therefore, this overwrites the NUL after the domain name and so\n> concatenates the two names. Good.\n>\n> I found this by dumping the values of the variables, because the\n> documentation of LookupAccountSid is not clear about the values that the\n> variables receive in the success case.\n>\n>> +\treturn translate_sid_to_user;\n>> +}\n>> +\n>\n> This patch looks good and works for me.\n>\n> Acked-by: Johannes Sixt \n>\n> Thank you!\n>\n> -- Hannes\n"},{"id":"486472","messageId":"e7594386-4a26-467d-bd27-8ac6268ad219@kdbg.org","threadId":"60669","inReplyTo":"xmqq8r4ygtkd.fsf@gitster.g","subject":"Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-01-09T21:05:39Z","receivedAt":"2024-01-09T21:05:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.01.24 um 21:06 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>>> +\tLookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,\n>>> +\t\t\t &pe_use); \n>>\n>> At this point, the function fails, so len_user and len_domain contain\n>> the required buffer size (including the trailing NUL).\n> \n> So (*str)[len_domain] would be the trailing NUL after the domain\n> part in the next call? Or would that be (*str)[len_domain-1]? I am\n> puzzled by off-by-one with your \"including the trailing NUL\" remark.\n\n\"Required buffer size\" must count the trailing NUL. So, the NUL would be\nat (*str)[len_domain-1].\n\n> \n>>> +\t/*\n>>> +\t * Alloc needed space of the strings\n>>> +\t */\n>>> +\tALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); \n> \n> This obviously assumes for domain 'd' and user 'u', we want \"d/u\" and\n> len_domain must be 1+1 (including NUL) and len_user must be 1+1\n> (including NUL). But then ...\n\nSo, this allocates the exact amount that is required to contain the\nnames with the trailing NULs: 1+1 plus 1+1 in this example.\n\n> \n>>> +\ttranslate_sid_to_user = LookupAccountSidA(NULL, sid,\n>>> +\t (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);\n> \n> ... ((*str)+len_domain) is presumably the beginning of the user\n> part, and (*str)+0) is where the domain part is to be stored.\n\nCorrect.\n\n> \n> Because len_domain includes the terminating NUL for the domain part,\n> (*str)[len_domain-1] is that NUL, no? And that is what you want to\n> overwrite to make the two strings into a single\n> one . So...\n\nBut after a successful call, len_domain and len_user have been modified\nto contain the lengths of the names (not counting the NULs), so, here\nthe NUL is at (*str)[len_domain]...\n\n> \n>> At this point, if the function is successful, len_user and len_domain\n>> contain the lengths of the names (without the trailing NUL).\n>>\n>>> +\tif (!translate_sid_to_user)\n>>> +\t\tFREE_AND_NULL(*str);\n>>> +\telse\n>>> +\t\t(*str)[len_domain] = '/';\n> \n> ... this offset looks fishy to me. Am I off-by-one?\n\n... and this offset is correct.\n\nI followed the same train of thought and suspected an off-by-one error,\ntoo, and was perplexed that I see a correct output. The documentation of\nLookupAccountSid is unclear that the variables change values across the\n(successful) call, but my tests verified that the change does happen.\n\n>> Therefore, this overwrites the NUL after the domain name and so\n>> concatenates the two names. Good.\n>>\n>> I found this by dumping the values of the variables, because the\n>> documentation of LookupAccountSid is not clear about the values that the\n>> variables receive in the success case.\n>>\n>>> +\treturn translate_sid_to_user;\n>>> +}\n>>> +\n>>\n>> This patch looks good and works for me.\n>>\n>> Acked-by: Johannes Sixt \n>>\n>> Thank you!\n>>\n>> -- Hannes\n\n-- Hannes\n\n"},{"id":"486477","messageId":"xmqq4jfmf85d.fsf@gitster.g","threadId":"60669","inReplyTo":"e7594386-4a26-467d-bd27-8ac6268ad219@kdbg.org","subject":"Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T22:34:38Z","receivedAt":"2024-01-09T22:34:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> Because len_domain includes the terminating NUL for the domain part,\n>> (*str)[len_domain-1] is that NUL, no? And that is what you want to\n>> overwrite to make the two strings into a single\n>> one . So...\n>\n> But after a successful call, len_domain and len_user have been modified\n> to contain the lengths of the names (not counting the NULs), ...\n\nAh, that is what I missed. Thanks.\n"}]}