{"thread":{"id":"58709","subject":"[PATCH] adjust_shared_perm(): leave g+s alone when the group does not matter","startedAt":"2022-10-28T21:16:37Z","lastAt":"2022-10-28T22:50:04Z","messageCount":5,"participants":["Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465983","messageId":"xmqqr0yrhco6.fsf@gitster.g","threadId":"58709","inReplyTo":null,"subject":"[PATCH] adjust_shared_perm(): leave g+s alone when the group does not matter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-28T21:16:09Z","receivedAt":"2022-10-28T21:16:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Julien Moutinho reports that in an environment where directory does\nnot have BSD group semantics and requires g+s (aka FORCE_DIR_SET_GID)\nbut the system cripples chmod() to forbid g+s, adjust_shared_perm()\nfails even when the repository is for private use with perm = 0600.\n\nWhen we grant extra access based on group membership (i.e. the\ndirectory has either g+r or g+w bit set), which group the directory\nand its contents are owned by matters. But otherwise (e.g. perm is\nset to 0600, in Julien's case), flipping g+s bit is not necessary.\n\nReported-by: Julien Moutinho \nSigned-off-by: Junio C Hamano \n---\n path.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/path.c b/path.c\nindex a3cfcd8a6e..492e17ad12 100644\n--- a/path.c\n+++ b/path.c\n@@ -901,7 +901,13 @@ int adjust_shared_perm(const char *path)\n \tif (S_ISDIR(old_mode)) {\n \t\t/* Copy read bits to execute bits */\n \t\tnew_mode |= (new_mode & 0444) >> 2;\n-\t\tnew_mode |= FORCE_DIR_SET_GID;\n+\n+\t\t/*\n+\t\t * g+s matters only if any extra access is granted\n+\t\t * based on group membership.\n+\t\t */\n+\t\tif (FORCE_DIR_SET_GID && (new_mode & 060))\n+\t\t\tnew_mode |= FORCE_DIR_SET_GID;\n \t}\n \n \tif (((old_mode ^ new_mode) & ~S_IFMT) &&\n-- \n2.38.1-280-g63bba4fdd8\n\n"},{"id":"465985","messageId":"Y1xNrDB10XEcAa0d@tapette.crustytoothpaste.net","threadId":"58709","inReplyTo":"xmqqr0yrhco6.fsf@gitster.g","subject":"Re: [PATCH] adjust_shared_perm(): leave g+s alone when the group does not matter","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-10-28T21:46:20Z","receivedAt":"2022-10-28T21:46:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-10-28 at 21:16:09, Junio C Hamano wrote:\n> Julien Moutinho reports that in an environment where directory does\n> not have BSD group semantics and requires g+s (aka FORCE_DIR_SET_GID)\n> but the system cripples chmod() to forbid g+s, adjust_shared_perm()\n\nI would personally use a different verb here because I have the\nimpression it's offensive, at least when used as a noun. Perhaps\n\"limit\" or \"restrict\" might be more neutral, or we could pick another\nverb which expresses our displeasure at this design (maybe \"impair\"?)\nbut maybe is less likely to be emotionally charged or offend.\n\n> fails even when the repository is for private use with perm = 0600.\n> \n> When we grant extra access based on group membership (i.e. the\n> directory has either g+r or g+w bit set), which group the directory\n> and its contents are owned by matters. But otherwise (e.g. perm is\n> set to 0600, in Julien's case), flipping g+s bit is not necessary.\n\nExcept for my comment above, I think the patch here addresses the\nproposed issue and looks good, and as usual, is well explained.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"465987","messageId":"xmqqbkpvhb0x.fsf@gitster.g","threadId":"58709","inReplyTo":"Y1xNrDB10XEcAa0d@tapette.crustytoothpaste.net","subject":"Re: [PATCH] adjust_shared_perm(): leave g+s alone when the group does not matter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-28T21:51:42Z","receivedAt":"2022-10-28T21:51:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2022-10-28 at 21:16:09, Junio C Hamano wrote:\n>> Julien Moutinho reports that in an environment where directory does\n>> not have BSD group semantics and requires g+s (aka FORCE_DIR_SET_GID)\n>> but the system cripples chmod() to forbid g+s, adjust_shared_perm()\n>\n> I would personally use a different verb here because I have the\n> impression it's offensive, at least when used as a noun. Perhaps\n> \"limit\" or \"restrict\" might be more neutral, or we could pick another\n> verb which expresses our displeasure at this design (maybe \"impair\"?)\n> but maybe is less likely to be emotionally charged or offend.\n\ncastrates? butchers?\n\ntweaks? That's quite neutral.\n\n"},{"id":"465989","messageId":"Y1xWBiZUAbPmt71W@tapette.crustytoothpaste.net","threadId":"58709","inReplyTo":"xmqqbkpvhb0x.fsf@gitster.g","subject":"Re: [PATCH] adjust_shared_perm(): leave g+s alone when the group does not matter","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-10-28T22:21:58Z","receivedAt":"2022-10-28T22:22:08Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-10-28 at 21:51:42, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > On 2022-10-28 at 21:16:09, Junio C Hamano wrote:\n> >> Julien Moutinho reports that in an environment where directory does\n> >> not have BSD group semantics and requires g+s (aka FORCE_DIR_SET_GID)\n> >> but the system cripples chmod() to forbid g+s, adjust_shared_perm()\n> >\n> > I would personally use a different verb here because I have the\n> > impression it's offensive, at least when used as a noun. Perhaps\n> > \"limit\" or \"restrict\" might be more neutral, or we could pick another\n> > verb which expresses our displeasure at this design (maybe \"impair\"?)\n> > but maybe is less likely to be emotionally charged or offend.\n> \n> castrates? butchers?\n> \n> tweaks? That's quite neutral.\n\nI think \"butchers\" or \"tweaks\" should be fine. I might say \"modifies\"\nas well.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"465991","messageId":"xmqq5yg3ftri.fsf@gitster.g","threadId":"58709","inReplyTo":"Y1xWBiZUAbPmt71W@tapette.crustytoothpaste.net","subject":"Re: [PATCH] adjust_shared_perm(): leave g+s alone when the group does not matter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-28T22:49:53Z","receivedAt":"2022-10-28T22:50:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2022-10-28 at 21:51:42, Junio C Hamano wrote:\n>> \"brian m. carlson\" writes:\n>> \n>> > On 2022-10-28 at 21:16:09, Junio C Hamano wrote:\n>> >> Julien Moutinho reports that in an environment where directory does\n>> >> not have BSD group semantics and requires g+s (aka FORCE_DIR_SET_GID)\n>> >> but the system cripples chmod() to forbid g+s, adjust_shared_perm()\n>> >\n>> > I would personally use a different verb here because I have the\n>> > impression it's offensive, at least when used as a noun. Perhaps\n>> > \"limit\" or \"restrict\" might be more neutral, or we could pick another\n>> > verb which expresses our displeasure at this design (maybe \"impair\"?)\n>> > but maybe is less likely to be emotionally charged or offend.\n>> \n>> castrates? butchers?\n>> \n>> tweaks? That's quite neutral.\n>\n> I think \"butchers\" or \"tweaks\" should be fine. I might say \"modifies\"\n> as well.\n\nI've decided to weaken it a lot by phrasing it like so:\n\n ... but the system forbids chmod() to touch the g+s bit, ...\n\n\n\n"}]}