{"thread":{"id":"58149","subject":"[PATCH 0/3] Some improvements to safe.directory on Windows","startedAt":"2022-07-13T08:21:37Z","lastAt":"2022-08-09T08:59:22Z","messageCount":16,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"458939","messageId":"pull.1286.git.1657700238.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":null,"subject":"[PATCH 0/3] Some improvements to safe.directory on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-13T08:17:14Z","receivedAt":"2022-07-13T08:21:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Due to the semantics being substantially different from Unix, the\nsafe.directory feature presents its own set of problems on Windows. One\nparticular issue would have prevented it from working in GitHub Actions'\nbuild agents, which we definitely rely on in the Git project itself. This\nwas addressed via the first two patches, which had made it already into Git\nfor Windows v2.35.2, and they are ready to be applied to core Git, too.\n\nThe third patch came in later, and was released as part of Git for Windows\nv2.37.0, so I also have confidence that it is stable and ready to be\nintegrated into core Git, too.\n\nJohannes Schindelin (3):\n Allow debugging unsafe directories' ownership\n mingw: handle a file owned by the Administrators group correctly\n mingw: be more informative when ownership check fails on FAT32\n\n Documentation/config/safe.txt | 6 ++++\n compat/mingw.c | 53 +++++++++++++++++++++++++++++++++++\n setup.c | 14 +++++++--\n 3 files changed, 71 insertions(+), 2 deletions(-)\n\n\nbase-commit: 3b0bf2704980b1ed6018622bdf5377ec22289688\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1286%2Fdscho%2Fsafe.directory-and-windows-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1286/dscho/safe.directory-and-windows-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1286\n-- \ngitgitgadget\n"},{"id":"458940","messageId":"3480381b8b99142bcc0213957a43d68a962c52d9.1657700238.git.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.git.1657700238.gitgitgadget@gmail.com","subject":"[PATCH 1/3] Allow debugging unsafe directories' ownership","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-13T08:17:15Z","receivedAt":"2022-07-13T08:21:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen Git refuses to use an existing repository because it is owned by\nsomeone else than the current user, it can be a bit tricky on Windows to\nfigure out what is going on.\n\nLet's help with that by offering some more information via the\nenvironment variable `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`.\n\nSigned-off-by: Johannes Schindelin \n---\n Documentation/config/safe.txt | 6 ++++++\n compat/mingw.c | 21 +++++++++++++++++++++\n setup.c | 14 ++++++++++++--\n 3 files changed, 39 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt\nindex 74627c5e7c6..18fac9cb7f3 100644\n--- a/Documentation/config/safe.txt\n+++ b/Documentation/config/safe.txt\n@@ -40,3 +40,9 @@ which id the original user has.\n If that is not what you would prefer and want git to only trust\n repositories that are owned by root instead, then you can remove\n the `SUDO_UID` variable from root's environment before invoking git.\n++\n+Due to the permission model on Windows where ACLs are used instead of\n+Unix' simpler permission model, it can be a bit tricky to figure out why\n+a directory is considered unsafe. To help with this, Git will provide\n+more detailed information when the environment variable\n+`GIT_TEST_DEBUG_UNSAFE_DIRECTORIES` is set to `true`.\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 38ac35913df..912444fb3ab 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1,6 +1,7 @@\n #include \"../git-compat-util.h\"\n #include \"win32.h\"\n #include \n+#include \n #include \n #include \n #include \"../strbuf.h\"\n@@ -2676,6 +2677,26 @@ int is_path_owned_by_current_sid(const char *path)\n \t\t IsValidSid(current_user_sid) &&\n \t\t EqualSid(sid, current_user_sid))\n \t\t\tresult = 1;\n+\t\telse if (git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0)) {\n+\t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n+\n+\t\t\tif (ConvertSidToStringSidA(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\telse\n+\t\t\t\tstr2 = \"(inconvertible)\";\n+\t\t\twarning(\"'%s' is owned by:\\n\\t'%s'\\nbut the current user is:\\n\\t'%s'\", path, str1, str2);\n+\t\t\tLocalFree(to_free1);\n+\t\t\tLocalFree(to_free2);\n+\t\t}\n \t}\n \n \t/*\ndiff --git a/setup.c b/setup.c\nindex 9dcecda65b0..3ba42ffcb27 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1353,13 +1353,23 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tcase GIT_DIR_INVALID_OWNERSHIP:\n \t\tif (!nongit_ok) {\n \t\t\tstruct strbuf quoted = STRBUF_INIT;\n+\t\t\tstruct strbuf hint = STRBUF_INIT;\n+\n+#ifdef __MINGW32__\n+\t\t\tif (!git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0))\n+\t\t\t\tstrbuf_addstr(&hint,\n+\t\t\t\t\t _(\"\\n\\nSet the environment variable \"\n+\t\t\t\t\t\t\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true \"\n+\t\t\t\t\t\t\"and run\\n\"\n+\t\t\t\t\t\t\"again for more information.\"));\n+#endif\n \n \t\t\tsq_quote_buf_pretty("ed, dir.buf);\n \t\t\tdie(_(\"detected dubious ownership in repository at '%s'\\n\"\n \t\t\t \"To add an exception for this directory, call:\\n\"\n \t\t\t \"\\n\"\n-\t\t\t \"\\tgit config --global --add safe.directory %s\"),\n-\t\t\t dir.buf, quoted.buf);\n+\t\t\t \"\\tgit config --global --add safe.directory %s%s\"),\n+\t\t\t dir.buf, quoted.buf, hint.buf);\n \t\t}\n \t\t*nongit_ok = 1;\n \t\tbreak;\n-- \ngitgitgadget\n\n"},{"id":"458941","messageId":"be06d711a1349a886cad63f8f2ec6675e6d5055e.1657700238.git.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.git.1657700238.gitgitgadget@gmail.com","subject":"[PATCH 2/3] mingw: handle a file owned by the Administrators group correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-13T08:17:16Z","receivedAt":"2022-07-13T08:21:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen an Administrator creates a file or directory, the created\nfile/directory is owned not by the Administrator SID, but by the\n_Administrators Group_ SID. The reason is that users with administrator\nprivileges usually run in unprivileged (\"non-elevated\") mode, and their\nuser SID does not change when running in elevated mode.\n\nThis is is relevant e.g. when running a GitHub workflow on a build\nagent, which runs in elevated mode: cloning a Git repository in a script\nstep will cause the worktree to be owned by the Administrators Group\nSID, for example.\n\nLet's handle this case as following: if the current user is an\nadministrator, Git should consider a worktree owned by the\nAdministrators Group as if it were owned by said user.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 912444fb3ab..e0e020ee574 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2669,6 +2669,7 @@ int is_path_owned_by_current_sid(const char *path)\n \telse if (sid && IsValidSid(sid)) {\n \t\t/* Now, verify that the SID matches the current user's */\n \t\tstatic PSID current_user_sid;\n+\t\tBOOL is_member;\n \n \t\tif (!current_user_sid)\n \t\t\tcurrent_user_sid = get_current_user_sid();\n@@ -2677,6 +2678,15 @@ int is_path_owned_by_current_sid(const char *path)\n \t\t IsValidSid(current_user_sid) &&\n \t\t EqualSid(sid, current_user_sid))\n \t\t\tresult = 1;\n+\t\telse if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) &&\n+\t\t\t CheckTokenMembership(NULL, sid, &is_member) &&\n+\t\t\t is_member)\n+\t\t\t/*\n+\t\t\t * If owned by the Administrators group, and the\n+\t\t\t * current user is an administrator, we consider that\n+\t\t\t * okay, too.\n+\t\t\t */\n+\t\t\tresult = 1;\n \t\telse if (git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0)) {\n \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n \n-- \ngitgitgadget\n\n"},{"id":"458942","messageId":"dae03f1b20430ff93c826b126057961a9030f2bf.1657700238.git.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.git.1657700238.gitgitgadget@gmail.com","subject":"[PATCH 3/3] mingw: be more informative when ownership check fails on FAT32","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-13T08:17:17Z","receivedAt":"2022-07-13T08:21:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe FAT file system has no concept of ACLs. Therefore, it cannot store\nany ownership information anyway, and the `GetNamedSecurityInfoW()` call\npretends that everything is owned \"by the world\".\n\nLet's special-case that scenario and tell the user what's going on, at\nleast when they set `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`.\n\nThis addresses https://github.com/git-for-windows/git/issues/3886\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 24 +++++++++++++++++++++++-\n 1 file changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex e0e020ee574..4144d6247bd 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2630,6 +2630,21 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+static int acls_supported(const char *path)\n+{\n+\tsize_t offset = offset_1st_component(path);\n+\tWCHAR wroot[MAX_PATH];\n+\tDWORD file_system_flags;\n+\n+\tif (offset &&\n+\t xutftowcsn(wroot, path, MAX_PATH, offset) > 0 &&\n+\t GetVolumeInformationW(wroot, NULL, 0, NULL, NULL,\n+\t\t\t\t &file_system_flags, NULL, 0))\n+\t\treturn !!(file_system_flags & FILE_PERSISTENT_ACLS);\n+\n+\treturn 0;\n+}\n+\n int is_path_owned_by_current_sid(const char *path)\n {\n \tWCHAR wpath[MAX_PATH];\n@@ -2687,7 +2702,14 @@ int is_path_owned_by_current_sid(const char *path)\n \t\t\t * okay, too.\n \t\t\t */\n \t\t\tresult = 1;\n-\t\telse if (git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0)) {\n+\t\telse if (IsWellKnownSid(sid, WinWorldSid) &&\n+\t\t\t git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0) &&\n+\t\t\t !acls_supported(path)) {\n+\t\t\t/*\n+\t\t\t * On FAT32 volumes, ownership is not actually recorded.\n+\t\t\t */\n+\t\t\twarning(\"'%s' is on a file system that does not record ownership\", path);\n+\t\t} else if (git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0)) {\n \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n \n \t\t\tif (ConvertSidToStringSidA(sid, &str1))\n-- \ngitgitgadget\n"},{"id":"459005","messageId":"xmqq5yk0dcvk.fsf@gitster.g","threadId":"58149","inReplyTo":"3480381b8b99142bcc0213957a43d68a962c52d9.1657700238.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] Allow debugging unsafe directories' ownership","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T19:35:43Z","receivedAt":"2022-07-13T19:35:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When Git refuses to use an existing repository because it is owned by\n> someone else than the current user, it can be a bit tricky on Windows to\n> figure out what is going on.\n>\n> Let's help with that by offering some more information via the\n> environment variable `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`.\n\nI can see how the change to compat/ part is useful, but ...\n\n> diff --git a/setup.c b/setup.c\n> index 9dcecda65b0..3ba42ffcb27 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1353,13 +1353,23 @@ const char *setup_git_directory_gently(int *nongit_ok)\n> \tcase GIT_DIR_INVALID_OWNERSHIP:\n> \t\tif (!nongit_ok) {\n> \t\t\tstruct strbuf quoted = STRBUF_INIT;\n> +\t\t\tstruct strbuf hint = STRBUF_INIT;\n> +\n> +#ifdef __MINGW32__\n> +\t\t\tif (!git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0))\n> +\t\t\t\tstrbuf_addstr(&hint,\n> +\t\t\t\t\t _(\"\\n\\nSet the environment variable \"\n> +\t\t\t\t\t\t\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true \"\n> +\t\t\t\t\t\t\"and run\\n\"\n> +\t\t\t\t\t\t\"again for more information.\"));\n> +#endif\n\n... I am not sure about this part. Do we have any other codepath to\nshow \"to debug, run the program with this\" suggestion? Adding it in\nthe documentation is probably good, but this is an extra message\nthat is much larger than the \"owned by X but you are Y\" message that\nwould be shown. With or without the environment set, the output\nwill become noisier with this patch. I wonder if we are better off\ngiving the information that is given in the warning (in compat/ part\nof the patch) _unconditionally_ in the message, which would make it\nless noisy overall.\n\nAlso what's our vision for non-Windows platform for \"further\ndebugging aid\" in this area? Would we perhaps want a similar\n\"warning\" that says \"owned by X but you are Y\"?\n\nThanks.\n"},{"id":"459094","messageId":"xmqqy1wv2x1n.fsf@gitster.g","threadId":"58149","inReplyTo":"xmqq5yk0dcvk.fsf@gitster.g","subject":"Re: [PATCH 1/3] Allow debugging unsafe directories' ownership","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-14T21:40:04Z","receivedAt":"2022-07-14T21:40:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... I am not sure about this part. Do we have any other codepath to\n> show \"to debug, run the program with this\" suggestion? Adding it in\n> the documentation is probably good, but this is an extra message\n> that is much larger than the \"owned by X but you are Y\" message that\n> would be shown. With or without the environment set, the output\n> will become noisier with this patch. I wonder if we are better off\n> giving the information that is given in the warning (in compat/ part\n> of the patch) _unconditionally_ in the message, which would make it\n> less noisy overall.\n\nI am wondering if passing a struct to allow is_path_owned_by*()\nhelper(s) to report the detail of the failures they discover a\ncleaner way to do this. To illustrate what I meant, along the\nlines of the following patch, with any additional code to actually\nstuff messages in the strbuf report, in the is_path_owned_by*() \nimplementation.\n\nI am perfectly OK if we make it a debug-only option by hiding this\nbehind an environment variable, but if we were to do so, I do not\nwant to see us advertize the environment variable in the die()\nmessage (a debug-only option can be documented, but not worth\nsurfacing in and contaminating the usual UI output).\n\nComments?\n\n compat/mingw.c | 2 +-\n git-compat-util.h | 3 ++-\n setup.c | 12 +++++++++---\n 3 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git c/compat/mingw.c w/compat/mingw.c\nindex 2607de93af..f12b7df16d 100644\n--- c/compat/mingw.c\n+++ w/compat/mingw.c\n@@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n-int is_path_owned_by_current_sid(const char *path)\n+int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n {\n \tWCHAR wpath[MAX_PATH];\n \tPSID sid = NULL;\ndiff --git c/git-compat-util.h w/git-compat-util.h\nindex 58d7708296..de34b0ea7e 100644\n--- c/git-compat-util.h\n+++ w/git-compat-util.h\n@@ -487,7 +487,8 @@ static inline void extract_id_from_env(const char *env, uid_t *id)\n \t}\n }\n \n-static inline int is_path_owned_by_current_uid(const char *path)\n+struct strbuf;\n+static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)\n {\n \tstruct stat st;\n \tuid_t euid;\ndiff --git c/setup.c w/setup.c\nindex 09b6549ba9..ed823585f7 100644\n--- c/setup.c\n+++ w/setup.c\n@@ -1143,12 +1143,18 @@ static int ensure_valid_ownership(const char *gitfile,\n \tstruct safe_directory_data data = {\n \t\t.path = worktree ? worktree : gitdir\n \t};\n+\tstruct strbuf report = STRBUF_INIT;\n \n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n-\t (!gitfile || is_path_owned_by_current_user(gitfile)) &&\n-\t (!worktree || is_path_owned_by_current_user(worktree)) &&\n-\t (!gitdir || is_path_owned_by_current_user(gitdir)))\n+\t (!gitfile || is_path_owned_by_current_user(gitfile, &report)) &&\n+\t (!worktree || is_path_owned_by_current_user(worktree, &report)) &&\n+\t (!gitdir || is_path_owned_by_current_user(gitdir, &report))) {\n+\t\tif (report.len) {\n+\t\t\tfputs(report.buf, stderr);\n+\t\t\tstrbuf_release(&report);\n+\t\t}\n \t\treturn 1;\n+\t}\n \n \t/*\n \t * data.path is the \"path\" that identifies the repository and it is\n"},{"id":"459149","messageId":"8rqqnqp1-q613-ron6-6q8s-n7sq57o980q9@tzk.qr","threadId":"58149","inReplyTo":"xmqqy1wv2x1n.fsf@gitster.g","subject":"Re: [PATCH 1/3] Allow debugging unsafe directories' ownership","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-07-15T14:33:53Z","receivedAt":"2022-07-15T14:34:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 14 Jul 2022, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > ... I am not sure about this part. Do we have any other codepath to\n> > show \"to debug, run the program with this\" suggestion? Adding it in\n> > the documentation is probably good, but this is an extra message\n> > that is much larger than the \"owned by X but you are Y\" message that\n> > would be shown. With or without the environment set, the output\n> > will become noisier with this patch. I wonder if we are better off\n> > giving the information that is given in the warning (in compat/ part\n> > of the patch) _unconditionally_ in the message, which would make it\n> > less noisy overall.\n>\n> I am wondering if passing a struct to allow is_path_owned_by*()\n> helper(s) to report the detail of the failures they discover a\n> cleaner way to do this. To illustrate what I meant, along the\n> lines of the following patch, with any additional code to actually\n> stuff messages in the strbuf report, in the is_path_owned_by*()\n> implementation.\n\nI like it! Let me play with this, after this coming week (during which I\nplan to be mostly offline).\n\nThanks,\nDscho\n\n>\n> I am perfectly OK if we make it a debug-only option by hiding this\n> behind an environment variable, but if we were to do so, I do not\n> want to see us advertize the environment variable in the die()\n> message (a debug-only option can be documented, but not worth\n> surfacing in and contaminating the usual UI output).\n>\n> Comments?\n>\n> compat/mingw.c | 2 +-\n> git-compat-util.h | 3 ++-\n> setup.c | 12 +++++++++---\n> 3 files changed, 12 insertions(+), 5 deletions(-)\n>\n> diff --git c/compat/mingw.c w/compat/mingw.c\n> index 2607de93af..f12b7df16d 100644\n> --- c/compat/mingw.c\n> +++ w/compat/mingw.c\n> @@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void)\n> \treturn result;\n> }\n>\n> -int is_path_owned_by_current_sid(const char *path)\n> +int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n> {\n> \tWCHAR wpath[MAX_PATH];\n> \tPSID sid = NULL;\n> diff --git c/git-compat-util.h w/git-compat-util.h\n> index 58d7708296..de34b0ea7e 100644\n> --- c/git-compat-util.h\n> +++ w/git-compat-util.h\n> @@ -487,7 +487,8 @@ static inline void extract_id_from_env(const char *env, uid_t *id)\n> \t}\n> }\n>\n> -static inline int is_path_owned_by_current_uid(const char *path)\n> +struct strbuf;\n> +static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)\n> {\n> \tstruct stat st;\n> \tuid_t euid;\n> diff --git c/setup.c w/setup.c\n> index 09b6549ba9..ed823585f7 100644\n> --- c/setup.c\n> +++ w/setup.c\n> @@ -1143,12 +1143,18 @@ static int ensure_valid_ownership(const char *gitfile,\n> \tstruct safe_directory_data data = {\n> \t\t.path = worktree ? worktree : gitdir\n> \t};\n> +\tstruct strbuf report = STRBUF_INIT;\n>\n> \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n> -\t (!gitfile || is_path_owned_by_current_user(gitfile)) &&\n> -\t (!worktree || is_path_owned_by_current_user(worktree)) &&\n> -\t (!gitdir || is_path_owned_by_current_user(gitdir)))\n> +\t (!gitfile || is_path_owned_by_current_user(gitfile, &report)) &&\n> +\t (!worktree || is_path_owned_by_current_user(worktree, &report)) &&\n> +\t (!gitdir || is_path_owned_by_current_user(gitdir, &report))) {\n> +\t\tif (report.len) {\n> +\t\t\tfputs(report.buf, stderr);\n> +\t\t\tstrbuf_release(&report);\n> +\t\t}\n> \t\treturn 1;\n> +\t}\n>\n> \t/*\n> \t * data.path is the \"path\" that identifies the repository and it is\n>\n"},{"id":"460818","messageId":"pull.1286.v2.git.1659965270.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.git.1657700238.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Some improvements to safe.directory on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T13:27:45Z","receivedAt":"2022-08-08T13:28:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Due to the semantics being substantially different from Unix, the\nsafe.directory feature presents its own set of problems on Windows. One\nparticular issue would have prevented it from working in GitHub Actions'\nbuild agents, which we definitely rely on in the Git project itself. This\nwas addressed via the fifth patch, which had made it (in a slightly\ndifferent form) already into Git for Windows v2.35.2, and they are ready to\nbe applied to core Git, too.\n\nThe FAT32 patch came in later, and was released as part of Git for Windows\nv2.37.0, so I also have confidence that it is stable and ready to be\nintegrated into core Git, too.\n\nChanges since v1:\n\n * Restructured the patch series.\n * Instead of an environment variable to turn on debugging, we now always\n show the platform-dependent information together with the error message\n about the dubious ownership (iff it is shown, that is), based on an idea\n by Junio.\n * Rebased onto gc/bare-repo-discovery to avoid a merge conflict.\n\nJohannes Schindelin (5):\n setup: fix some formatting\n Prepare for more detailed \"dubious ownership\" messages\n mingw: provide details about unsafe directories' ownership\n mingw: be more informative when ownership check fails on FAT32\n mingw: handle a file owned by the Administrators group correctly\n\n compat/mingw.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++-\n compat/mingw.h | 2 +-\n git-compat-util.h | 5 +++-\n setup.c | 30 ++++++++++++++----------\n 4 files changed, 81 insertions(+), 15 deletions(-)\n\n\nbase-commit: 776f184893d2861a729aa4b91d69931036e03e4b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1286%2Fdscho%2Fsafe.directory-and-windows-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1286/dscho/safe.directory-and-windows-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1286\n\nRange-diff vs v1:\n\n -: ----------- > 1: 301d94f18f5 setup: fix some formatting\n -: ----------- > 2: 8cc45e4922a Prepare for more detailed \"dubious ownership\" messages\n 1: 3480381b8b9 ! 3: 63494818105 Allow debugging unsafe directories' ownership\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - Allow debugging unsafe directories' ownership\n + mingw: provide details about unsafe directories' ownership\n \n When Git refuses to use an existing repository because it is owned by\n someone else than the current user, it can be a bit tricky on Windows to\n figure out what is going on.\n \n - Let's help with that by offering some more information via the\n - environment variable `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`.\n + Let's help with that by providing more detailed information.\n \n Signed-off-by: Johannes Schindelin \n \n - ## Documentation/config/safe.txt ##\n -@@ Documentation/config/safe.txt: which id the original user has.\n - If that is not what you would prefer and want git to only trust\n - repositories that are owned by root instead, then you can remove\n - the `SUDO_UID` variable from root's environment before invoking git.\n -++\n -+Due to the permission model on Windows where ACLs are used instead of\n -+Unix' simpler permission model, it can be a bit tricky to figure out why\n -+a directory is considered unsafe. To help with this, Git will provide\n -+more detailed information when the environment variable\n -+`GIT_TEST_DEBUG_UNSAFE_DIRECTORIES` is set to `true`.\n -\n ## compat/mingw.c ##\n @@\n #include \"../git-compat-util.h\"\n @@ compat/mingw.c\n #include \n #include \n #include \"../strbuf.h\"\n -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)\n +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t IsValidSid(current_user_sid) &&\n \t\t EqualSid(sid, current_user_sid))\n \t\t\tresult = 1;\n -+\t\telse if (git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0)) {\n ++\t\telse if (report) {\n +\t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n +\n +\t\t\tif (ConvertSidToStringSidA(sid, &str1))\n @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)\n +\t\t\t\tto_free2 = str2;\n +\t\t\telse\n +\t\t\t\tstr2 = \"(inconvertible)\";\n -+\t\t\twarning(\"'%s' is owned by:\\n\\t'%s'\\nbut the current user is:\\n\\t'%s'\", path, str1, str2);\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}\n \t}\n \n \t/*\n -\n - ## setup.c ##\n -@@ setup.c: const char *setup_git_directory_gently(int *nongit_ok)\n - \tcase GIT_DIR_INVALID_OWNERSHIP:\n - \t\tif (!nongit_ok) {\n - \t\t\tstruct strbuf quoted = STRBUF_INIT;\n -+\t\t\tstruct strbuf hint = STRBUF_INIT;\n -+\n -+#ifdef __MINGW32__\n -+\t\t\tif (!git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0))\n -+\t\t\t\tstrbuf_addstr(&hint,\n -+\t\t\t\t\t _(\"\\n\\nSet the environment variable \"\n -+\t\t\t\t\t\t\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true \"\n -+\t\t\t\t\t\t\"and run\\n\"\n -+\t\t\t\t\t\t\"again for more information.\"));\n -+#endif\n - \n - \t\t\tsq_quote_buf_pretty("ed, dir.buf);\n - \t\t\tdie(_(\"detected dubious ownership in repository at '%s'\\n\"\n - \t\t\t \"To add an exception for this directory, call:\\n\"\n - \t\t\t \"\\n\"\n --\t\t\t \"\\tgit config --global --add safe.directory %s\"),\n --\t\t\t dir.buf, quoted.buf);\n -+\t\t\t \"\\tgit config --global --add safe.directory %s%s\"),\n -+\t\t\t dir.buf, quoted.buf, hint.buf);\n - \t\t}\n - \t\t*nongit_ok = 1;\n - \t\tbreak;\n 3: dae03f1b204 ! 4: 7aaa6248dfe mingw: be more informative when ownership check fails on FAT32\n @@ Commit message\n any ownership information anyway, and the `GetNamedSecurityInfoW()` call\n pretends that everything is owned \"by the world\".\n \n - Let's special-case that scenario and tell the user what's going on, at\n - least when they set `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`.\n + Let's special-case that scenario and tell the user what's going on.\n \n This addresses https://github.com/git-for-windows/git/issues/3886\n \n @@ compat/mingw.c: static PSID get_current_user_sid(void)\n +\treturn 0;\n +}\n +\n - int is_path_owned_by_current_sid(const char *path)\n + int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n {\n \tWCHAR wpath[MAX_PATH];\n -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)\n - \t\t\t * okay, too.\n - \t\t\t */\n +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n + \t\t IsValidSid(current_user_sid) &&\n + \t\t EqualSid(sid, current_user_sid))\n \t\t\tresult = 1;\n --\t\telse if (git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0)) {\n -+\t\telse if (IsWellKnownSid(sid, WinWorldSid) &&\n -+\t\t\t git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0) &&\n +-\t\telse if (report) {\n ++\t\telse if (report &&\n ++\t\t\t IsWellKnownSid(sid, WinWorldSid) &&\n +\t\t\t !acls_supported(path)) {\n +\t\t\t/*\n +\t\t\t * On FAT32 volumes, ownership is not actually recorded.\n +\t\t\t */\n -+\t\t\twarning(\"'%s' is on a file system that does not record ownership\", path);\n -+\t\t} else if (git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0)) {\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 \n \t\t\tif (ConvertSidToStringSidA(sid, &str1))\n 2: be06d711a13 ! 5: fbfaff2ec21 mingw: handle a file owned by the Administrators group correctly\n @@ Commit message\n Signed-off-by: Johannes Schindelin \n \n ## compat/mingw.c ##\n -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)\n +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \telse if (sid && IsValidSid(sid)) {\n \t\t/* Now, verify that the SID matches the current user's */\n \t\tstatic PSID current_user_sid;\n @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)\n \n \t\tif (!current_user_sid)\n \t\t\tcurrent_user_sid = get_current_user_sid();\n -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)\n +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t IsValidSid(current_user_sid) &&\n \t\t EqualSid(sid, current_user_sid))\n \t\t\tresult = 1;\n @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)\n +\t\t\t * okay, too.\n +\t\t\t */\n +\t\t\tresult = 1;\n - \t\telse if (git_env_bool(\"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES\", 0)) {\n - \t\t\tLPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;\n - \n + \t\telse if (report &&\n + \t\t\t IsWellKnownSid(sid, WinWorldSid) &&\n + \t\t\t !acls_supported(path)) {\n\n-- \ngitgitgadget\n"},{"id":"460819","messageId":"301d94f18f501b44b6b96fd7c170134d4661be38.1659965270.git.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.v2.git.1659965270.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] setup: fix some formatting","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T13:27:46Z","receivedAt":"2022-08-08T13:28:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn preparation for touching code that was introduced in 3b0bf2704980\n(setup: tighten ownership checks post CVE-2022-24765, 2022-05-10) and\nthat was formatted differently than preferred in the Git project, fix\nthe indentation before actually modifying the code.\n\nSigned-off-by: Johannes Schindelin \n---\n setup.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 8c683e92b62..2f7b3e598f8 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1142,7 +1142,7 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n * added, for bare ones their git directory.\n */\n static int ensure_valid_ownership(const char *gitfile,\n-\t\t\t\tconst char *worktree, const char *gitdir)\n+\t\t\t\t const char *worktree, const char *gitdir)\n {\n \tstruct safe_directory_data data = {\n \t\t.path = worktree ? worktree : gitdir\n@@ -1316,10 +1316,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tstrbuf_setlen(dir, offset);\n \t\tif (gitdirenv) {\n \t\t\tenum discovery_result ret;\n+\t\t\tconst char *gitdir_candidate =\n+\t\t\t\tgitdir_path ? gitdir_path : gitdirenv;\n \n-\t\t\tif (ensure_valid_ownership(gitfile,\n-\t\t\t\t\t\t dir->buf,\n-\t\t\t\t (gitdir_path ? gitdir_path : gitdirenv))) {\n+\t\t\tif (ensure_valid_ownership(gitfile, dir->buf,\n+\t\t\t\t\t\t gitdir_candidate)) {\n \t\t\t\tstrbuf_addstr(gitdir, gitdirenv);\n \t\t\t\tret = GIT_DIR_DISCOVERED;\n \t\t\t} else\n-- \ngitgitgadget\n\n"},{"id":"460820","messageId":"8cc45e4922a6a445e9f643e709c3efaf48553a09.1659965270.git.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.v2.git.1659965270.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] Prepare for more detailed \"dubious ownership\" messages","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T13:27:47Z","receivedAt":"2022-08-08T13:28:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen verifying the ownership of the Git directory, we sometimes would\nlike to say a bit more about it, e.g. when using a platform-dependent\ncode path (think: Windows and the permission model that is so different\nfrom Unix'), but only when it is a appropriate to actually say\nsomething.\n\nTo allow for that, collect that information and hand it back to the\ncaller (whose responsibility it is to show it or not).\n\nNote: We do not actually fill in any platform-dependent information yet,\nthis commit just adds the infrastructure to be able to do so.\n\nBased-on-an-idea-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 2 +-\n compat/mingw.h | 2 +-\n git-compat-util.h | 5 ++++-\n setup.c | 25 +++++++++++++++----------\n 4 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 2607de93af5..f12b7df16d9 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n-int is_path_owned_by_current_sid(const char *path)\n+int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n {\n \tWCHAR wpath[MAX_PATH];\n \tPSID sid = NULL;\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex a74da68f313..209cf7cebad 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -463,7 +463,7 @@ char *mingw_query_user_email(void);\n * Verifies that the specified path is owned by the user running the\n * current process.\n */\n-int is_path_owned_by_current_sid(const char *path);\n+int is_path_owned_by_current_sid(const char *path, struct strbuf *report);\n #define is_path_owned_by_current_user is_path_owned_by_current_sid\n \n /**\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 58d7708296b..36a25ae252f 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -23,6 +23,9 @@\n #include \n #endif\n \n+struct strbuf;\n+\n+\n #define _FILE_OFFSET_BITS 64\n \n \n@@ -487,7 +490,7 @@ static inline void extract_id_from_env(const char *env, uid_t *id)\n \t}\n }\n \n-static inline int is_path_owned_by_current_uid(const char *path)\n+static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)\n {\n \tstruct stat st;\n \tuid_t euid;\ndiff --git a/setup.c b/setup.c\nindex 2f7b3e598f8..cefd5f63c46 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1142,16 +1142,17 @@ static int safe_directory_cb(const char *key, const char *value, void *d)\n * added, for bare ones their git directory.\n */\n static int ensure_valid_ownership(const char *gitfile,\n-\t\t\t\t const char *worktree, const char *gitdir)\n+\t\t\t\t const char *worktree, const char *gitdir,\n+\t\t\t\t struct strbuf *report)\n {\n \tstruct safe_directory_data data = {\n \t\t.path = worktree ? worktree : gitdir\n \t};\n \n \tif (!git_env_bool(\"GIT_TEST_ASSUME_DIFFERENT_OWNER\", 0) &&\n-\t (!gitfile || is_path_owned_by_current_user(gitfile)) &&\n-\t (!worktree || is_path_owned_by_current_user(worktree)) &&\n-\t (!gitdir || is_path_owned_by_current_user(gitdir)))\n+\t (!gitfile || is_path_owned_by_current_user(gitfile, report)) &&\n+\t (!worktree || is_path_owned_by_current_user(worktree, report)) &&\n+\t (!gitdir || is_path_owned_by_current_user(gitdir, report)))\n \t\treturn 1;\n \n \t/*\n@@ -1232,6 +1233,7 @@ enum discovery_result {\n */\n static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\t\t\t\t struct strbuf *gitdir,\n+\t\t\t\t\t\t\t struct strbuf *report,\n \t\t\t\t\t\t\t int die_on_error)\n {\n \tconst char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);\n@@ -1320,7 +1322,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\tgitdir_path ? gitdir_path : gitdirenv;\n \n \t\t\tif (ensure_valid_ownership(gitfile, dir->buf,\n-\t\t\t\t\t\t gitdir_candidate)) {\n+\t\t\t\t\t\t gitdir_candidate, report)) {\n \t\t\t\tstrbuf_addstr(gitdir, gitdirenv);\n \t\t\t\tret = GIT_DIR_DISCOVERED;\n \t\t\t} else\n@@ -1345,7 +1347,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tif (is_git_directory(dir->buf)) {\n \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n-\t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf))\n+\t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n \t\t\tstrbuf_addstr(gitdir, \".\");\n \t\t\treturn GIT_DIR_BARE;\n@@ -1378,7 +1380,7 @@ int discover_git_directory(struct strbuf *commondir,\n \t\treturn -1;\n \n \tcwd_len = dir.len;\n-\tif (setup_git_directory_gently_1(&dir, gitdir, 0) <= 0) {\n+\tif (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {\n \t\tstrbuf_release(&dir);\n \t\treturn -1;\n \t}\n@@ -1425,7 +1427,7 @@ int discover_git_directory(struct strbuf *commondir,\n const char *setup_git_directory_gently(int *nongit_ok)\n {\n \tstatic struct strbuf cwd = STRBUF_INIT;\n-\tstruct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;\n+\tstruct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT, report = STRBUF_INIT;\n \tconst char *prefix = NULL;\n \tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \n@@ -1450,7 +1452,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tdie_errno(_(\"Unable to read current working directory\"));\n \tstrbuf_addbuf(&dir, &cwd);\n \n-\tswitch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {\n+\tswitch (setup_git_directory_gently_1(&dir, &gitdir, &report, 1)) {\n \tcase GIT_DIR_EXPLICIT:\n \t\tprefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);\n \t\tbreak;\n@@ -1482,12 +1484,14 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tif (!nongit_ok) {\n \t\t\tstruct strbuf quoted = STRBUF_INIT;\n \n+\t\t\tstrbuf_complete(&report, '\\n');\n \t\t\tsq_quote_buf_pretty("ed, dir.buf);\n \t\t\tdie(_(\"detected dubious ownership in repository at '%s'\\n\"\n+\t\t\t \"%s\"\n \t\t\t \"To add an exception for this directory, call:\\n\"\n \t\t\t \"\\n\"\n \t\t\t \"\\tgit config --global --add safe.directory %s\"),\n-\t\t\t dir.buf, quoted.buf);\n+\t\t\t dir.buf, report.buf, quoted.buf);\n \t\t}\n \t\t*nongit_ok = 1;\n \t\tbreak;\n@@ -1574,6 +1578,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \n \tstrbuf_release(&dir);\n \tstrbuf_release(&gitdir);\n+\tstrbuf_release(&report);\n \tclear_repository_format(&repo_fmt);\n \n \treturn prefix;\n-- \ngitgitgadget\n\n"},{"id":"460821","messageId":"6349481810585d4dba2187266510d71fb2471cb7.1659965270.git.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.v2.git.1659965270.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] mingw: provide details about unsafe directories' ownership","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T13:27:48Z","receivedAt":"2022-08-08T13:28:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen Git refuses to use an existing repository because it is owned by\nsomeone else than the current user, it can be a bit tricky on Windows to\nfigure out what is going on.\n\nLet's help with that by providing more detailed information.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex f12b7df16d9..2c09c5bffee 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1,6 +1,7 @@\n #include \"../git-compat-util.h\"\n #include \"win32.h\"\n #include \n+#include \n #include \n #include \n #include \"../strbuf.h\"\n@@ -2720,6 +2721,29 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t IsValidSid(current_user_sid) &&\n \t\t EqualSid(sid, current_user_sid))\n \t\t\tresult = 1;\n+\t\telse 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\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\telse\n+\t\t\t\tstr2 = \"(inconvertible)\";\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}\n \t}\n \n \t/*\n-- \ngitgitgadget\n\n"},{"id":"460822","messageId":"7aaa6248dfef7fb8436b5fbccf4689d0be3226a5.1659965270.git.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.v2.git.1659965270.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] mingw: be more informative when ownership check fails on FAT32","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T13:27:49Z","receivedAt":"2022-08-08T13:28:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe FAT file system has no concept of ACLs. Therefore, it cannot store\nany ownership information anyway, and the `GetNamedSecurityInfoW()` call\npretends that everything is owned \"by the world\".\n\nLet's special-case that scenario and tell the user what's going on.\n\nThis addresses https://github.com/git-for-windows/git/issues/3886\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 25 ++++++++++++++++++++++++-\n 1 file changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 2c09c5bffee..22f960c7e34 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2674,6 +2674,21 @@ static PSID get_current_user_sid(void)\n \treturn result;\n }\n \n+static int acls_supported(const char *path)\n+{\n+\tsize_t offset = offset_1st_component(path);\n+\tWCHAR wroot[MAX_PATH];\n+\tDWORD file_system_flags;\n+\n+\tif (offset &&\n+\t xutftowcsn(wroot, path, MAX_PATH, offset) > 0 &&\n+\t GetVolumeInformationW(wroot, NULL, 0, NULL, NULL,\n+\t\t\t\t &file_system_flags, NULL, 0))\n+\t\treturn !!(file_system_flags & FILE_PERSISTENT_ACLS);\n+\n+\treturn 0;\n+}\n+\n int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n {\n \tWCHAR wpath[MAX_PATH];\n@@ -2721,7 +2736,15 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t IsValidSid(current_user_sid) &&\n \t\t EqualSid(sid, current_user_sid))\n \t\t\tresult = 1;\n-\t\telse if (report) {\n+\t\telse if (report &&\n+\t\t\t IsWellKnownSid(sid, WinWorldSid) &&\n+\t\t\t !acls_supported(path)) {\n+\t\t\t/*\n+\t\t\t * On FAT32 volumes, ownership is not actually recorded.\n+\t\t\t */\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 \n \t\t\tif (ConvertSidToStringSidA(sid, &str1))\n-- \ngitgitgadget\n\n"},{"id":"460823","messageId":"fbfaff2ec21d76229eb4d86f6bfe4eab4de20c96.1659965270.git.gitgitgadget@gmail.com","threadId":"58149","inReplyTo":"pull.1286.v2.git.1659965270.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] mingw: handle a file owned by the Administrators group correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T13:27:50Z","receivedAt":"2022-08-08T13:28:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen an Administrator creates a file or directory, the created\nfile/directory is owned not by the Administrator SID, but by the\n_Administrators Group_ SID. The reason is that users with administrator\nprivileges usually run in unprivileged (\"non-elevated\") mode, and their\nuser SID does not change when running in elevated mode.\n\nThis is is relevant e.g. when running a GitHub workflow on a build\nagent, which runs in elevated mode: cloning a Git repository in a script\nstep will cause the worktree to be owned by the Administrators Group\nSID, for example.\n\nLet's handle this case as following: if the current user is an\nadministrator, Git should consider a worktree owned by the\nAdministrators Group as if it were owned by said user.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 22f960c7e34..7aa9318db72 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2728,6 +2728,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \telse if (sid && IsValidSid(sid)) {\n \t\t/* Now, verify that the SID matches the current user's */\n \t\tstatic PSID current_user_sid;\n+\t\tBOOL is_member;\n \n \t\tif (!current_user_sid)\n \t\t\tcurrent_user_sid = get_current_user_sid();\n@@ -2736,6 +2737,15 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n \t\t IsValidSid(current_user_sid) &&\n \t\t EqualSid(sid, current_user_sid))\n \t\t\tresult = 1;\n+\t\telse if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) &&\n+\t\t\t CheckTokenMembership(NULL, sid, &is_member) &&\n+\t\t\t is_member)\n+\t\t\t/*\n+\t\t\t * If owned by the Administrators group, and the\n+\t\t\t * current user is an administrator, we consider that\n+\t\t\t * okay, too.\n+\t\t\t */\n+\t\t\tresult = 1;\n \t\telse if (report &&\n \t\t\t IsWellKnownSid(sid, WinWorldSid) &&\n \t\t\t !acls_supported(path)) {\n-- \ngitgitgadget\n"},{"id":"460824","messageId":"14q05972-5507-21n5-p61o-qq62366n6nsp@tzk.qr","threadId":"58149","inReplyTo":"8rqqnqp1-q613-ron6-6q8s-n7sq57o980q9@tzk.qr","subject":"Re: [PATCH 1/3] Allow debugging unsafe directories' ownership","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-08T13:29:58Z","receivedAt":"2022-08-08T13:29:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 15 Jul 2022, Johannes Schindelin wrote:\n\n> On Thu, 14 Jul 2022, Junio C Hamano wrote:\n>\n> > Junio C Hamano writes:\n> >\n> > > ... I am not sure about this part. Do we have any other codepath to\n> > > show \"to debug, run the program with this\" suggestion? Adding it in\n> > > the documentation is probably good, but this is an extra message\n> > > that is much larger than the \"owned by X but you are Y\" message that\n> > > would be shown. With or without the environment set, the output\n> > > will become noisier with this patch. I wonder if we are better off\n> > > giving the information that is given in the warning (in compat/ part\n> > > of the patch) _unconditionally_ in the message, which would make it\n> > > less noisy overall.\n> >\n> > I am wondering if passing a struct to allow is_path_owned_by*()\n> > helper(s) to report the detail of the failures they discover a\n> > cleaner way to do this. To illustrate what I meant, along the\n> > lines of the following patch, with any additional code to actually\n> > stuff messages in the strbuf report, in the is_path_owned_by*()\n> > implementation.\n>\n> I like it! Let me play with this, after this coming week (during which I\n> plan to be mostly offline).\n\nI had to play with it for a while until I could make it work, but\neventually I managed to do it. The second iteration was just sent, and\nimplements this route.\n\nThank you,\nDscho\n"},{"id":"460833","messageId":"xmqqzggesnbh.fsf@gitster.g","threadId":"58149","inReplyTo":"pull.1286.v2.git.1659965270.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Some improvements to safe.directory on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-08T16:38:10Z","receivedAt":"2022-08-08T16:38:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Due to the semantics being substantially different from Unix, the\n> safe.directory feature presents its own set of problems on Windows. One\n> particular issue would have prevented it from working in GitHub Actions'\n> build agents, which we definitely rely on in the Git project itself. This\n> was addressed via the fifth patch, which had made it (in a slightly\n> different form) already into Git for Windows v2.35.2, and they are ready to\n> be applied to core Git, too.\n>\n> The FAT32 patch came in later, and was released as part of Git for Windows\n> v2.37.0, so I also have confidence that it is stable and ready to be\n> integrated into core Git, too.\n>\n> Changes since v1:\n>\n> * Restructured the patch series.\n> * Instead of an environment variable to turn on debugging, we now always\n> show the platform-dependent information together with the error message\n> about the dubious ownership (iff it is shown, that is), based on an idea\n> by Junio.\n> * Rebased onto gc/bare-repo-discovery to avoid a merge conflict.\n\nI actually had to rebase it back so that we could merge it to\n'maint' for further 2.37.x releases. I'll refer to the original\npatches in this thread when I merge the result to 'seen', of course,\nto make sure the results do match. It would have been slightly less\nconvenient if you did not do this rebase, but it would have allowed\nme to have much better confidence in the result that may eventually\ngo to 'maint'. After all, mistakes in resolving merge conflicts on\n'seen' can be corrected before the topic hits 'next'.\n\nThanks. I do not know about the API calls mingw.c part of these\npatches make, but the overall structure looks sensible to me.\n\n"},{"id":"460880","messageId":"2q2nos2r-9151-6s87-q173-3r738n0rp452@tzk.qr","threadId":"58149","inReplyTo":"xmqqzggesnbh.fsf@gitster.g","subject":"Re: [PATCH v2 0/5] Some improvements to safe.directory on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-09T08:59:03Z","receivedAt":"2022-08-09T08:59:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 8 Aug 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > Due to the semantics being substantially different from Unix, the\n> > safe.directory feature presents its own set of problems on Windows. One\n> > particular issue would have prevented it from working in GitHub Actions'\n> > build agents, which we definitely rely on in the Git project itself. This\n> > was addressed via the fifth patch, which had made it (in a slightly\n> > different form) already into Git for Windows v2.35.2, and they are ready to\n> > be applied to core Git, too.\n> >\n> > The FAT32 patch came in later, and was released as part of Git for Windows\n> > v2.37.0, so I also have confidence that it is stable and ready to be\n> > integrated into core Git, too.\n> >\n> > Changes since v1:\n> >\n> > * Restructured the patch series.\n> > * Instead of an environment variable to turn on debugging, we now always\n> > show the platform-dependent information together with the error message\n> > about the dubious ownership (iff it is shown, that is), based on an idea\n> > by Junio.\n> > * Rebased onto gc/bare-repo-discovery to avoid a merge conflict.\n>\n> I actually had to rebase it back so that we could merge it to\n> 'maint' for further 2.37.x releases.\n\nI appreciate the effort you put into it, even if it is not your\nresponsibility to take care of Git for Windows' releases.\n\nThe range-diff shows that you snuck in a commit message change that I\nwould have either not made at all (I think the original was fine) or would\nhave made differently (because Access Control Lists are not specific to\nWindows, even if Windows is the most obvious example for this permission\nmodel):\n\n-- snip --\n1: 301d94f18f5 ! 1: d51e1dff980 setup: fix some formatting\n @@ Commit message\n the indentation before actually modifying the code.\n\n Signed-off-by: Johannes Schindelin \n + Signed-off-by: Junio C Hamano \n\n ## setup.c ##\n @@ setup.c: static int safe_directory_cb(const char *key, const char *value, void *d)\n2: 8cc45e4922a ! 2: 17d3883fe9c Prepare for more detailed \"dubious ownership\" messages\n @@ Metadata\n Author: Johannes Schindelin \n\n ## Commit message ##\n - Prepare for more detailed \"dubious ownership\" messages\n + setup: prepare for more detailed \"dubious ownership\" messages\n\n When verifying the ownership of the Git directory, we sometimes would\n like to say a bit more about it, e.g. when using a platform-dependent\n - code path (think: Windows and the permission model that is so different\n + code path (think: Windows has the permission model that is so different\n from Unix'), but only when it is a appropriate to actually say\n something.\n\n @@ Commit message\n\n Based-on-an-idea-by: Junio C Hamano \n Signed-off-by: Johannes Schindelin \n + Signed-off-by: Junio C Hamano \n\n ## compat/mingw.c ##\n @@ compat/mingw.c: static PSID get_current_user_sid(void)\n @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf\n \t\t\t\tret = GIT_DIR_DISCOVERED;\n \t\t\t} else\n @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n + \t\t}\n +\n \t\tif (is_git_directory(dir->buf)) {\n - \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n - \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n -\t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf))\n +\t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n3: 63494818105 ! 3: e883e04b68b mingw: provide details about unsafe directories' ownership\n @@ Commit message\n Let's help with that by providing more detailed information.\n\n Signed-off-by: Johannes Schindelin \n + Signed-off-by: Junio C Hamano \n\n ## compat/mingw.c ##\n @@\n4: 7aaa6248dfe ! 4: 7c83470e64e mingw: be more informative when ownership check fails on FAT32\n @@ Commit message\n This addresses https://github.com/git-for-windows/git/issues/3886\n\n Signed-off-by: Johannes Schindelin \n + Signed-off-by: Junio C Hamano \n\n ## compat/mingw.c ##\n @@ compat/mingw.c: static PSID get_current_user_sid(void)\n5: fbfaff2ec21 ! 5: 3f7207e2ea9 mingw: handle a file owned by the Administrators group correctly\n @@ Commit message\n Administrators Group as if it were owned by said user.\n\n Signed-off-by: Johannes Schindelin \n + Signed-off-by: Junio C Hamano \n\n ## compat/mingw.c ##\n @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)\n-- snap --\n\nRetitling the commit message was okay, of course, using the `setup:`\nprefix.\n\n> I'll refer to the original patches in this thread when I merge the\n> result to 'seen', of course, to make sure the results do match. It\n> would have been slightly less convenient if you did not do this rebase,\n> but it would have allowed me to have much better confidence in the\n> result that may eventually go to 'maint'. After all, mistakes in\n> resolving merge conflicts on 'seen' can be corrected before the topic\n> hits 'next'.\n\nYes, mistakes can happen, and the more people work together the easier it\nis to avoid or fix them.\n\n> Thanks. I do not know about the API calls mingw.c part of these\n> patches make, but the overall structure looks sensible to me.\n\nI do not think that anybody expected you to know about Win32 API calls ;-)\n\nCiao,\nDscho\n"}]}