{"thread":{"id":"18516","subject":"[BUG?] How to make a shared/restricted repo?","startedAt":"2009-03-25T00:05:05Z","lastAt":"2009-03-29T20:31:32Z","messageCount":30,"participants":["Johan Herland","Brandon Casey","Junio C Hamano","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"109272","messageId":"200903250105.05808.johan@herland.net","threadId":"18516","inReplyTo":null,"subject":"[BUG?] How to make a shared/restricted repo?","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T00:05:05Z","receivedAt":"2009-03-25T00:05:05Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Hi,\n\nSome colleagues of mine are working on a \"secret\" project, and they want to \ncreate a central/server/integration repo that should be group-writable, but \nnot at all accessible to anybody outside the group (i.e. files should be \n0660 (\"-rw-rw----\"), dirs should be 0770 (\"drwxrws---\")).\n\nI started setting this up for them in the following manner:\n\n mkdir foo.git\n cd foo.git\n git init --bare --shared=group\n cd ..\n chgrp -R groupname foo.git\n chmod -R o-rwx foo.git\n\n...and everything looks good, initially...\n\nHowever, when I start pushing into this repo, the newly created files are \nreadable to everybody (files are 0664 (\"-rw-rw-r--\"), dirs are 0775 \n(\"drwxrwsr-x\")).\n\nInstead of \"git init --bare --shared=group\", I've tried using\n git init --bare --shared=0660\nand even\n git init --bare &&\n git config core.sharedRepository 0660\nbut the result is still the same.\n\nAfter reading the \"--shared\" section in the \"git init\" man page, this \nbehaviour is unexpected, and after reading the \"core.sharedRepository\" \nsection in the \"git config\" man page, the current behaviour is IMHO outright \n_wrong_. Quoting the \"git config\" man page:\n\n core.sharedRepository\n [...] When 0xxx, where 0xxx is an octal number, files in the repository\n will have this mode value. 0xxx will override user’s umask value, and\n thus, users with a safe umask (0077) can use this option. [...]\n\nAFAICS, even when I set \"core.sharedRepository\" to 0660, files are still \ncreated 0664, which is not what the documentation indicates.\n\nAre there other ways to create such shared-but-restricted repositories?\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109277","messageId":"sL3rt6iQWyznVMwP2SukD7BiuS1AVuqwVkMR4XSwA5SnK9TLmqyqAg@cipher.nrlssc.navy.mil","threadId":"18516","inReplyTo":"200903250105.05808.johan@herland.net","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-03-25T00:26:17Z","receivedAt":"2009-03-25T00:26:17Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Johan Herland wrote:\n> Hi,\n> \n> Some colleagues of mine are working on a \"secret\" project, and they want to \n> create a central/server/integration repo that should be group-writable, but \n> not at all accessible to anybody outside the group (i.e. files should be \n> 0660 (\"-rw-rw----\"), dirs should be 0770 (\"drwxrws---\")).\n> \n> I started setting this up for them in the following manner:\n> \n> mkdir foo.git\n> cd foo.git\n> git init --bare --shared=group\n> cd ..\n> chgrp -R groupname foo.git\n> chmod -R o-rwx foo.git\n> \n> ...and everything looks good, initially...\n> \n> However, when I start pushing into this repo, the newly created files are \n> readable to everybody (files are 0664 (\"-rw-rw-r--\"), dirs are 0775 \n> (\"drwxrwsr-x\")).\n\nBut nobody has access to anything under foo.git since you did\n'chmod o-rwx foo.git' above.\n\nUnless I'm missing something, I think you already have what you want.\n\n-brandon\n"},{"id":"109281","messageId":"200903250145.51386.johan@herland.net","threadId":"18516","inReplyTo":"sL3rt6iQWyznVMwP2SukD7BiuS1AVuqwVkMR4XSwA5SnK9TLmqyqAg@cipher.nrlssc.navy.mil","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T00:45:51Z","receivedAt":"2009-03-25T00:45:51Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wednesday 25 March 2009, Brandon Casey wrote:\n> Johan Herland wrote:\n> > Some colleagues of mine are working on a \"secret\" project, and they\n> > want to create a central/server/integration repo that should be\n> > group-writable, but not at all accessible to anybody outside the group\n> > (i.e. files should be 0660 (\"-rw-rw----\"), dirs should be 2770\n> > (\"drwxrws---\")).\n> >\n> > I started setting this up for them in the following manner:\n> >\n> > mkdir foo.git\n> > cd foo.git\n> > git init --bare --shared=group\n> > cd ..\n> > chgrp -R groupname foo.git\n> > chmod -R o-rwx foo.git\n> >\n> > ...and everything looks good, initially...\n> >\n> > However, when I start pushing into this repo, the newly created files\n> > are readable to everybody (files are 0664 (\"-rw-rw-r--\"), dirs are 2775\n> > (\"drwxrwsr-x\")).\n>\n> But nobody has access to anything under foo.git since you did\n> 'chmod o-rwx foo.git' above.\n\nYes, it's hard (impossible???) for outside users to get at the files, since \nthey reside in directories that are not readable to them. However, this does \nnot at all hide the fact that:\n\n1. The \"chmod -R o-rwx\" is a command I added myself. Nowhere in Git's \ndocumentation is it said that it is a good idea to run this command.\n\n2. Preferably, when creating a 0660 repo, \"git init\" should automatically \nperform this chmod for you, in the same manner that it already sets the \n\"set-gid\" bit for group-shared repos.\n\n> Unless I'm missing something, I think you already have what you want.\n\nMaybe, but it certainly doesn't fill me with warm, fuzzy, secure feelings.\n\nAm I being overly paranoid?\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109282","messageId":"7vvdpybf9i.fsf@gitster.siamese.dyndns.org","threadId":"18516","inReplyTo":"200903250105.05808.johan@herland.net","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-25T00:46:49Z","receivedAt":"2009-03-25T00:46:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> core.sharedRepository\n> [...] When 0xxx, where 0xxx is an octal number, files in the repository\n> will have this mode value. 0xxx will override user’s umask value, and\n> thus, users with a safe umask (0077) can use this option. [...]\n\nTraditionally, core.shreadrepository was used only to widen overtight\numask some paranoid users have that are antisocial in a group project\nsetting. An object that happens to have created by a lenient member may\nbe readable by others, but another object created by a member with\nstricter umask won't be, which means \"core.sharedRepository = group\" means\njust that; it guarantees that it is at least usable by everybody in the\ngroup even if there is somebody antisocial in the group, and it does not\nsay anything about accessibility by others at all.\n\nYou might like to try a patch like this (untested).\n\n path.c | 17 +++++------------\n 1 files changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex e332b50..fbc7011 100644\n--- a/path.c\n+++ b/path.c\n@@ -314,7 +314,7 @@ char *enter_repo(char *path, int strict)\n int adjust_shared_perm(const char *path)\n {\n \tstruct stat st;\n-\tint mode;\n+\tint mode, tweak;\n \n \tif (!shared_repository)\n \t\treturn 0;\n@@ -322,17 +322,10 @@ int adjust_shared_perm(const char *path)\n \t\treturn -1;\n \tmode = st.st_mode;\n \n-\tif (shared_repository) {\n-\t\tint tweak = shared_repository;\n-\t\tif (!(mode & S_IWUSR))\n-\t\t\ttweak &= ~0222;\n-\t\tmode |= tweak;\n-\t} else {\n-\t\t/* Preserve old PERM_UMASK behaviour */\n-\t\tif (mode & S_IWUSR)\n-\t\t\tmode |= S_IWGRP;\n-\t}\n-\n+\ttweak = shared_repository;\n+\tif (!(mode & S_IWUSR))\n+\t\ttweak &= ~0222;\n+\tmode = (mode & ~0777) | tweak;\n \tif (S_ISDIR(mode)) {\n \t\tmode |= FORCE_DIR_SET_GID;\n \n"},{"id":"109283","messageId":"7vr60mbf4v.fsf@gitster.siamese.dyndns.org","threadId":"18516","inReplyTo":"sL3rt6iQWyznVMwP2SukD7BiuS1AVuqwVkMR4XSwA5SnK9TLmqyqAg@cipher.nrlssc.navy.mil","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-25T00:49:36Z","receivedAt":"2009-03-25T00:49:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Johan Herland wrote:\n>> Hi,\n>> \n>> Some colleagues of mine are working on a \"secret\" project, and they want to \n>> create a central/server/integration repo that should be group-writable, but \n>> not at all accessible to anybody outside the group (i.e. files should be \n>> 0660 (\"-rw-rw----\"), dirs should be 0770 (\"drwxrws---\")).\n>> \n>> I started setting this up for them in the following manner:\n>> \n>> mkdir foo.git\n>> cd foo.git\n>> git init --bare --shared=group\n>> cd ..\n>> chgrp -R groupname foo.git\n>> chmod -R o-rwx foo.git\n>> \n>> ...and everything looks good, initially...\n>> \n>> However, when I start pushing into this repo, the newly created files are \n>> readable to everybody (files are 0664 (\"-rw-rw-r--\"), dirs are 0775 \n>> (\"drwxrwsr-x\")).\n>\n> But nobody has access to anything under foo.git since you did\n> 'chmod o-rwx foo.git' above.\n>\n> Unless I'm missing something, I think you already have what you want.\n\nThe toplevel is never recreated so it should be Ok in practice.\n\nThe core.sharedrepository only loosens the effect of overtight umask\nsetting that a project member has. But you can notice inconsistency when\nyou run \"ls -l\", which may bother you ;-)\n"},{"id":"109291","messageId":"200903250311.56300.johan@herland.net","threadId":"18516","inReplyTo":"7vvdpybf9i.fsf@gitster.siamese.dyndns.org","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T02:11:56Z","receivedAt":"2009-03-25T02:11:56Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wednesday 25 March 2009, Junio C Hamano wrote:\n> You might like to try a patch like this (untested).\n>\n> path.c | 17 +++++------------\n> 1 files changed, 5 insertions(+), 12 deletions(-)\n\nThanks!\n\nThis works much better :)\n\nHowever, there are still some questions/issues:\n\n- t1301-shared-repo.sh fails:\n Oops, .git/HEAD is not 0664 but -rw-rw---- [...]\n * FAIL 3: shared=1 does not clear bits preset by umask 022\n (I guess this is expected, as your patch changes the assumptions)\n\n- Loose objects and pack files are still world-readable.\n\n- Shared repos are no longer world-readable by default (requires\n \"--shared=all\" to be world-readable). This might be\n confusing to old users with lenient umasks, and should probably be\n mentioned in the documentation. AFAICS, it also has nasty effects\n on existing repos with core.sharedRepository == 1. We should probably\n re-roll the patch adding a new keyword (e.g. \"group-only\"), instead of\n changing the semantics of existing keywords/modes.\n\n- Files/dirs copied from template directory are still world-readable.\n (This is not a big deal at all)\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109294","messageId":"7v63hybaqd.fsf@gitster.siamese.dyndns.org","threadId":"18516","inReplyTo":"200903250311.56300.johan@herland.net","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-25T02:24:42Z","receivedAt":"2009-03-25T02:24:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Wednesday 25 March 2009, Junio C Hamano wrote:\n>> You might like to try a patch like this (untested).\n>>\n>> path.c | 17 +++++------------\n>> 1 files changed, 5 insertions(+), 12 deletions(-)\n>\n> Thanks!\n>\n> This works much better :)\n>\n> However, there are still some questions/issues:\n>\n> - t1301-shared-repo.sh fails:\n> Oops, .git/HEAD is not 0664 but -rw-rw---- [...]\n> * FAIL 3: shared=1 does not clear bits preset by umask 022\n> (I guess this is expected, as your patch changes the assumptions)\n\nI'd rather say the patch breaks people's expectations.\n"},{"id":"109423","messageId":"200903252236.03010.johan@herland.net","threadId":"18516","inReplyTo":"7v63hybaqd.fsf@gitster.siamese.dyndns.org","subject":"[PATCH/RFC 0/7] Restricting repository access (Was: [BUG?] How to make a shared/restricted repo?)","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T21:36:02Z","receivedAt":"2009-03-25T21:36:02Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wednesday 25 March 2009, Junio C Hamano wrote:\n> Johan Herland writes:\n> > On Wednesday 25 March 2009, Junio C Hamano wrote:\n> >> You might like to try a patch like this (untested).\n> >>\n> >> path.c | 17 +++++------------\n> >> 1 files changed, 5 insertions(+), 12 deletions(-)\n> >\n> > Thanks!\n> >\n> > This works much better :)\n> >\n> > However, there are still some questions/issues:\n> >\n> > - t1301-shared-repo.sh fails:\n> > Oops, .git/HEAD is not 0664 but -rw-rw---- [...]\n> > * FAIL 3: shared=1 does not clear bits preset by umask 022\n> > (I guess this is expected, as your patch changes the assumptions)\n>\n> I'd rather say the patch breaks people's expectations.\n\nI thought some more about the current semantics, and came up with this\npatch series, which replaces your original suggestion.\n\nIn short, I leave the core.sharedRepository semantics as is (i.e. it is\nonly used to _loosen_ repository permissions), and introduce a new\nvariable - core.restrictedRepository - that takes care of _tightening_\nrepository permissions. Its value is a permission mask that is applied\nto the file mode in adjust_shared_perm()\n\nThe patch series is based on recent 'next', and the testsuite passes\nafter each individual patch.\n\nHere is a short rundown of the individual patches:\n\n1. Clarify existing documentation to reflect the current semantics of\n core.sharedRepository and \"git init --shared\". Even if the rest of\n the series is rejected, I hope this can make it in some form.\n\n2. Minor cleanup in path.c:adjust_shared_perm(). This is pretty much\n your original patch with any functional changes removed.\n\n3. Introduce core.restrictedRepository. Adds git_config_perm_mask()\n for parsing the config value, and changes adjust_shared_perm() to\n apply the permission mask. Includes documentation of the new config\n variable.\n\n4. Add \"--restricted\" to \"git init\". Heavily modeled on the existing\n \"--shared\" option. Includes documentation of the new option.\n\n5. Add tests for the functionality introduced in #3 and #4.\n\n6. Apply adjusted repository permissions in \"git init\" when copying\n templates into the new repo.\n\n7. Apply restricted permissions to loose objects and pack files. This\n ensures that loose objects and pack files do not get permissions\n that are more liberal than the rest of the repository.\n\n\nHave fun!\n\n...Johan\n\n\nJohan Herland (7):\n Clarify documentation on permissions in shared repositories\n Cleanup: Remove unnecessary if-else clause\n Introduce core.restrictedRepository for restricting repository\n permissions\n git-init: Introduce --restricted for restricting repository access\n Add tests for \"core.restrictedRepository\" and \"git init --restricted\"\n git-init: Apply correct mode bits to template files in\n shared/restricted repo\n Apply restricted permissions to loose objects and pack files\n\n Documentation/config.txt | 41 ++++++++++++-\n Documentation/git-init.txt | 50 +++++++++++++++--\n builtin-init-db.c | 31 +++++++++-\n cache.h | 8 +++\n environment.c | 1 +\n fast-import.c | 4 +-\n http-push.c | 2 +-\n http-walker.c | 2 +-\n index-pack.c | 4 +-\n path.c | 22 +++----\n setup.c | 36 ++++++++++++\n sha1_file.c | 2 +-\n t/t0001-init.sh | 24 +++++++-\n t/t1304-restricted-repo.sh | 132 ++++++++++++++++++++++++++++++++++++++++++++\n 14 files changed, 323 insertions(+), 36 deletions(-)\n create mode 100755 t/t1304-restricted-repo.sh\n"},{"id":"109424","messageId":"200903252237.29367.johan@herland.net","threadId":"18516","inReplyTo":"200903252236.03010.johan@herland.net","subject":"[PATCH/RFC 1/7] Clarify documentation on permissions in shared repositories","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T21:37:29Z","receivedAt":"2009-03-25T21:37:29Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"The \"core.sharedRepository\" config variable (and, by extension, the\n\"--shared\" argument to \"git init\") can be used to loosen repository\npermissions for users with a safe umask, but it can not be used to\ntighten repository permissions for users with a more lenient umask.\n\nThis patch updates the documentation to clarify the current behaviour.\n\nSigned-off-by: Johan Herland \n---\n Documentation/config.txt | 15 +++++++++++----\n Documentation/git-init.txt | 12 ++++++++----\n 2 files changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 089569a..d5befd5 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -294,10 +294,17 @@ core.sharedRepository::\n \trepository will be readable by all users, additionally to being\n \tgroup-shareable. When 'umask' (or 'false'), git will use permissions\n \treported by umask(2). When '0xxx', where '0xxx' is an octal number,\n-\tfiles in the repository will have this mode value. '0xxx' will override\n-\tuser's umask value, and thus, users with a safe umask (0077) can use\n-\tthis option. Examples: '0660' is equivalent to 'group'. '0640' is a\n-\trepository that is group-readable but not group-writable.\n+\tfiles in the repository will have (at least) this mode value. '0xxx'\n+\twill override a safer umask value, and thus, users with a safe umask\n+\t(0077) can use this option to loosen the repository permissions.\n+\tExamples: '0660' is equivalent to 'group'. '0640' is a repository\n+\tthat is group-readable but not group-writable (unless umask allows\n+\tgroup-writability).\n+\tNote: Even when not set to 'umask' (or 'false') this option is still\n+\tcombined with the umask to produce the actual mode value. For\n+\texample, if umask is 0022, setting 'group' (or '0660') will not make\n+\tthe repository non world-readable (the actual mode value will in fact\n+\tbe '0664').\n \tSee linkgit:git-init[1]. False by default.\n \n core.warnAmbiguousRefs::\ndiff --git a/Documentation/git-init.txt b/Documentation/git-init.txt\nindex 71749c0..bddc01b 100644\n--- a/Documentation/git-init.txt\n+++ b/Documentation/git-init.txt\n@@ -45,6 +45,7 @@ repository. When specified, the config variable \"core.sharedRepository\" is\n set so that files and directories under `$GIT_DIR` are created with the\n requested permissions. When not specified, git will use permissions reported\n by umask(2).\n+When specified, the permissions will still be no stricter than the umask.\n \n The option can have the following values, defaulting to 'group' if no value\n is given:\n@@ -58,11 +59,14 @@ is given:\n - 'all' (or 'world' or 'everybody'): Same as 'group', but make the repository\n readable by all users.\n \n- - '0xxx': '0xxx' is an octal number and each file will have mode '0xxx'\n+ - '0xxx': '0xxx' is an octal number and each file will have (at least) this\n+ mode value.\n Any option except 'umask' can be set using this option. '0xxx' will\n- override users umask(2) value, and thus, users with a safe umask (0077)\n- can use this option. '0640' will create a repository which is group-readable\n- but not writable. '0660' is equivalent to 'group'.\n+ override a safer umask(2) value (but not a more permissive umask), and\n+ thus, users with a safe umask (e.g. 0077) can use this option to loosen\n+ repository permissions. '0640' will create a repository which is\n+ group-readable but not writable (unless umask allows group-writability).\n+ '0660' is equivalent to 'group'.\n \n By default, the configuration flag receive.denyNonFastForwards is enabled\n in shared repositories, so that you cannot force a non fast-forwarding push\n-- \n1.6.2.1.473.g92672\n"},{"id":"109425","messageId":"200903252238.30928.johan@herland.net","threadId":"18516","inReplyTo":"200903252236.03010.johan@herland.net","subject":"[PATCH/RFC 2/7] Cleanup: Remove unnecessary if-else clause","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T21:38:30Z","receivedAt":"2009-03-25T21:38:30Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Signed-off-by: Johan Herland \n---\n path.c | 14 ++++----------\n 1 files changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex e332b50..28e50d3 100644\n--- a/path.c\n+++ b/path.c\n@@ -322,16 +322,10 @@ int adjust_shared_perm(const char *path)\n \t\treturn -1;\n \tmode = st.st_mode;\n \n-\tif (shared_repository) {\n-\t\tint tweak = shared_repository;\n-\t\tif (!(mode & S_IWUSR))\n-\t\t\ttweak &= ~0222;\n-\t\tmode |= tweak;\n-\t} else {\n-\t\t/* Preserve old PERM_UMASK behaviour */\n-\t\tif (mode & S_IWUSR)\n-\t\t\tmode |= S_IWGRP;\n-\t}\n+\tint tweak = shared_repository;\n+\tif (!(mode & S_IWUSR))\n+\t\ttweak &= ~0222;\n+\tmode |= tweak;\n \n \tif (S_ISDIR(mode)) {\n \t\tmode |= FORCE_DIR_SET_GID;\n-- \n1.6.2.1.473.g92672\n"},{"id":"109426","messageId":"200903252239.16594.johan@herland.net","threadId":"18516","inReplyTo":"200903252236.03010.johan@herland.net","subject":"[PATCH/RFC 3/7] Introduce core.restrictedRepository for restricting repository permissions","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T21:39:16Z","receivedAt":"2009-03-25T21:39:16Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"core.sharedRepository is only used to loosen/expand permissions (beyond the\nuser's umask). In order to tighten/restrict permissions (stricter than the\numask), we add a new configuration variable that serves as an additional\nmask on the permissions of created files and directories.\n\nThe new config variable - core.restrictedRepository - can be set to an octal\nnumber, interpreted as an additional mask to be applied to the permissions\nof created files and directories. It can also be set to \"umask\" (or \"false\",\nequivalent to \"0000\", i.e. no more restrictive than the user's umask), \"user\"\n(or \"true\", equivalent to \"0077\", i.e. dropping read- and write-permissions\nfor group and others), or \"group\" (equivalent to \"0007\", i.e. dropping read-\nand write-permissions for others). If set with no value, it defaults to\n\"user\" (the most secure/restricted state).\n\nSigned-off-by: Johan Herland \n---\n Documentation/config.txt | 26 ++++++++++++++++++++++++++\n cache.h | 7 +++++++\n environment.c | 1 +\n path.c | 14 ++++++++------\n setup.c | 36 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 78 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex d5befd5..0f2dd5c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -305,8 +305,34 @@ core.sharedRepository::\n \texample, if umask is 0022, setting 'group' (or '0660') will not make\n \tthe repository non world-readable (the actual mode value will in fact\n \tbe '0664').\n+\tSee \"core.restrictedRepository\" for how to restrict repository access\n+\tbelow/within the umask.\n \tSee linkgit:git-init[1]. False by default.\n \n+core.restrictedRepository::\n+\tWhen 'user' (or 'true'), the repository is made inaccessible to\n+\tanybody but the user owning the repository. When 'group', repository\n+\taccess is restricted to the members of the repository's group.\n+\tWhen 'umask' (or 'false'), git will use permissions reported by\n+\tumask(2). When '0xxx', where '0xxx' is an octal number, files in the\n+\trepository will the specified mode bits masked off. '0xxx' will\n+\toverride a more lenient umask value, and thus, users with a loose\n+\tumask (e.g. 0022) can use this option to further restrict the\n+\trepository permissions.\n+\tExamples: '0007' is equivalent to 'group'. '0077' is a repository\n+\tthat is only accessible to the repository owner.\n+\tNote: Even when not set to 'umask' (or 'false') this option is still\n+\tcombined with the umask to produce the actual mask value. For\n+\texample, if umask is 0077, setting 'group' (or '0007') will not make\n+\tthe repository group-accessible (the calculated mode mask will in\n+\tfact be '0077', restricting access to the repository owner only).\n+\tSee \"core.sharedRepository\" for how to loosen repository access\n+\tbeyond the umask.\n+\tExample: To set up a group-shared repository that is inaccessible to\n+\tall non-members, set both \"core.sharedRepository\" and\n+\t\"core.restrictedRepository\" to \"group\".\n+\tFalse by default.\n+\n core.warnAmbiguousRefs::\n \tIf true, git will warn you if the ref name you passed it is ambiguous\n \tand might match multiple refs in the .git/refs/ tree. True by default.\ndiff --git a/cache.h b/cache.h\nindex 9cf5a13..4730f33 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -508,6 +508,7 @@ extern int prefer_symlink_refs;\n extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int shared_repository;\n+extern int restricted_repository;\n extern const char *apply_default_whitespace;\n extern int zlib_compression_level;\n extern int core_compression_level;\n@@ -622,7 +623,13 @@ enum sharedrepo {\n \tPERM_GROUP = 0660,\n \tPERM_EVERYBODY = 0664,\n };\n+enum restrictedrepo {\n+\tPERM_MASK_UMASK = 0000,\n+\tPERM_MASK_GROUP = 0007,\n+\tPERM_MASK_USER = 0077,\n+};\n int git_config_perm(const char *var, const char *value);\n+int git_config_perm_mask(const char *var, const char *value);\n int adjust_shared_perm(const char *path);\n int safe_create_leading_directories(char *path);\n int safe_create_leading_directories_const(const char *path);\ndiff --git a/environment.c b/environment.c\nindex 4696885..9591710 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -25,6 +25,7 @@ int repository_format_version;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\n int shared_repository = PERM_UMASK;\n+int restricted_repository = PERM_MASK_UMASK;\n const char *apply_default_whitespace;\n int zlib_compression_level = Z_BEST_SPEED;\n int core_compression_level;\ndiff --git a/path.c b/path.c\nindex 28e50d3..093c87c 100644\n--- a/path.c\n+++ b/path.c\n@@ -316,25 +316,27 @@ int adjust_shared_perm(const char *path)\n \tstruct stat st;\n \tint mode;\n \n-\tif (!shared_repository)\n+\tif (!shared_repository && !restricted_repository)\n \t\treturn 0;\n \tif (lstat(path, &st) < 0)\n \t\treturn -1;\n \tmode = st.st_mode;\n \n-\tint tweak = shared_repository;\n \tif (!(mode & S_IWUSR))\n-\t\ttweak &= ~0222;\n-\tmode |= tweak;\n+\t\trestricted_repository |= 0222;\n+\n+\t/* add shared_repository and subtract restricted_repository */\n+\tmode |= shared_repository;\n+\tmode &= ~restricted_repository;\n \n \tif (S_ISDIR(mode)) {\n \t\tmode |= FORCE_DIR_SET_GID;\n \n \t\t/* Copy read bits to execute bits */\n-\t\tmode |= (shared_repository & 0444) >> 2;\n+\t\tmode |= (mode & 0444) >> 2;\n \t}\n \n-\tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n+\tif (st.st_mode != mode && chmod(path, mode) < 0)\n \t\treturn -2;\n \treturn 0;\n }\ndiff --git a/setup.c b/setup.c\nindex 6c2deda..97685d4 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -459,12 +459,48 @@ int git_config_perm(const char *var, const char *value)\n \treturn i & 0666;\n }\n \n+int git_config_perm_mask(const char *var, const char *value)\n+{\n+\tint i;\n+\tchar *endptr;\n+\n+\tif (value == NULL)\n+\t\treturn PERM_MASK_USER;\n+\n+\tif (!strcmp(value, \"umask\"))\n+\t\treturn PERM_MASK_UMASK;\n+\tif (!strcmp(value, \"group\"))\n+\t\treturn PERM_MASK_GROUP;\n+\tif (!strcmp(value, \"user\"))\n+\t\treturn PERM_MASK_USER;\n+\n+\t/* Parse octal numbers */\n+\ti = strtol(value, &endptr, 8);\n+\n+\t/* If not an octal number, maybe true/false? */\n+\tif (*endptr != 0)\n+\t\treturn git_config_bool(var, value) ? PERM_MASK_USER :\n+\t\t\tPERM_MASK_UMASK;\n+\n+\t/* A filemode mask value was given: 0xxx */\n+\n+\tif (i & 0700)\n+\t\tdie(\"Problem with core.restrictedRepository filemode mask \"\n+\t\t \"value (0%.3o).\\nThe owner of files must always have \"\n+\t\t \"read and write permissions.\", i);\n+\n+\t/* Mask filemode mask value. Should only affect r/w/x flags for g/o */\n+\treturn i & 0077;\n+}\n+\n int check_repository_format_version(const char *var, const char *value, void *cb)\n {\n \tif (strcmp(var, \"core.repositoryformatversion\") == 0)\n \t\trepository_format_version = git_config_int(var, value);\n \telse if (strcmp(var, \"core.sharedrepository\") == 0)\n \t\tshared_repository = git_config_perm(var, value);\n+\telse if (strcmp(var, \"core.restrictedrepository\") == 0)\n+\t\trestricted_repository = git_config_perm_mask(var, value);\n \telse if (strcmp(var, \"core.bare\") == 0) {\n \t\tis_bare_repository_cfg = git_config_bool(var, value);\n \t\tif (is_bare_repository_cfg == 1)\n-- \n1.6.2.1.473.g92672\n"},{"id":"109427","messageId":"200903252239.53864.johan@herland.net","threadId":"18516","inReplyTo":"200903252236.03010.johan@herland.net","subject":"[PATCH/RFC 4/7] git-init: Introduce --restricted for restricting repository access","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T21:39:53Z","receivedAt":"2009-03-25T21:39:53Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"\"--restricted\" does for \"core.restrictedRepository\" what \"--shared\" does for\n\"core.sharedRepository\".\n\nSigned-off-by: Johan Herland \n---\n Documentation/config.txt | 2 +-\n Documentation/git-init.txt | 38 +++++++++++++++++++++++++++++++++++++-\n builtin-init-db.c | 22 ++++++++++++++++++----\n 3 files changed, 56 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0f2dd5c..08f8068 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -331,7 +331,7 @@ core.restrictedRepository::\n \tExample: To set up a group-shared repository that is inaccessible to\n \tall non-members, set both \"core.sharedRepository\" and\n \t\"core.restrictedRepository\" to \"group\".\n-\tFalse by default.\n+\tSee linkgit:git-init[1]. False by default.\n \n core.warnAmbiguousRefs::\n \tIf true, git will warn you if the ref name you passed it is ambiguous\ndiff --git a/Documentation/git-init.txt b/Documentation/git-init.txt\nindex bddc01b..2a431c2 100644\n--- a/Documentation/git-init.txt\n+++ b/Documentation/git-init.txt\n@@ -8,7 +8,7 @@ git-init - Create an empty git repository or reinitialize an existing one\n \n SYNOPSIS\n --------\n-'git init' [-q | --quiet] [--bare] [--template=] [--shared[=]]\n+'git init' [-q | --quiet] [--bare] [--template=] [--shared[=]] [--restricted[=]]\n \n \n OPTIONS\n@@ -72,6 +72,42 @@ By default, the configuration flag receive.denyNonFastForwards is enabled\n in shared repositories, so that you cannot force a non fast-forwarding push\n into it.\n \n+--restricted[={false|true|umask|group|user|0xxx}]::\n+\n+Specify that the git repository is to be restricted according to the given\n+permission mask. This allows you to more finely control access to the\n+repository. When specified, the config variable \"core.restrictedRepository\"\n+is set so that files and directories under `$GIT_DIR` are created with the\n+restrictions in the given mask. When not specified, git will use permissions\n+reported by umask(2). When specified, the permissions will still be no more\n+lenient than the umask allows.\n+\n+The option can have the following values, defaulting to 'user' if no value\n+is given:\n+\n+ - 'umask' (or 'false'): Use permissions reported by umask(2). The default,\n+ when `--restricted` is not specified.\n+\n+ - 'group': Make the repository accessible only to members of the group\n+ owning the repository.\n+\n+ - 'user' (or 'true'): Make the repository inaccessible to anybody but the\n+ repository owner.\n+\n+ - '0xxx': '0xxx' is an octal number and each file will have (at least) these\n+ mode bits masked off the repository permission. '0xxx' will override a\n+ more lenient umask(2) value (but not a stricter/safer umask), and thus,\n+ users with a lenient umask (e.g. 0022) can use this option to tighten\n+ repository permissions. '0000' is equivalent to 'umask', '0007' is\n+ equivalent to 'group', and '0077' is equivalent to 'user'.\n+ '0027' will create a repository which is group-readable (unless overridden\n+ by the current umask), but not group-writable, and inaccessible to others.\n+\n+You can combine `--shared` and `--restricted` to finely control the access to\n+the repository. For example, specifying `--shared=group --restricted=group`\n+will ensure that the repository is group-readable and group-writable, and\n+also non world-readable and non world-writable.\n+\n --\n \n \ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex fc63d0f..8e7fa2d 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -19,6 +19,7 @@\n \n static int init_is_bare_repository = 0;\n static int init_shared_repository = -1;\n+static int init_restricted_repository = -1;\n \n static void safe_create_dir(const char *dir, int share)\n {\n@@ -29,7 +30,7 @@ static void safe_create_dir(const char *dir, int share)\n \t\t}\n \t}\n \telse if (share && adjust_shared_perm(dir))\n-\t\tdie(\"Could not make %s writable by group\", dir);\n+\t\tdie(\"Could not set proper permissions on %s\", dir);\n }\n \n static void copy_templates_1(char *path, int baselen,\n@@ -196,12 +197,14 @@ static int create_default_files(const char *template_path)\n \tis_bare_repository_cfg = init_is_bare_repository;\n \tif (init_shared_repository != -1)\n \t\tshared_repository = init_shared_repository;\n+\tif (init_restricted_repository != -1)\n+\t\trestricted_repository = init_restricted_repository;\n \n \t/*\n \t * We would have created the above under user's umask -- under\n \t * shared-repository settings, we would need to fix them up.\n \t */\n-\tif (shared_repository) {\n+\tif (shared_repository || restricted_repository) {\n \t\tadjust_shared_perm(get_git_dir());\n \t\tadjust_shared_perm(git_path(\"refs\"));\n \t\tadjust_shared_perm(git_path(\"refs/heads\"));\n@@ -321,11 +324,17 @@ int init_db(const char *template_dir, unsigned int flags)\n \t\tgit_config_set(\"core.sharedrepository\", buf);\n \t\tgit_config_set(\"receive.denyNonFastforwards\", \"true\");\n \t}\n+\tif (restricted_repository) {\n+\t\tchar buf[5];\n+\t\tsprintf(buf, \"%04o\", restricted_repository);\n+\t\tgit_config_set(\"core.restrictedrepository\", buf);\n+\t}\n \n \tif (!(flags & INIT_DB_QUIET))\n-\t\tprintf(\"%s%s Git repository in %s/\\n\",\n+\t\tprintf(\"%s%s%s Git repository in %s/\\n\",\n \t\t reinit ? \"Reinitialized existing\" : \"Initialized empty\",\n \t\t shared_repository ? \" shared\" : \"\",\n+\t\t restricted_repository ? \" restricted\" : \"\",\n \t\t get_git_dir());\n \n \treturn 0;\n@@ -363,7 +372,7 @@ static int guess_repository_type(const char *git_dir)\n }\n \n static const char init_db_usage[] =\n-\"git init [-q | --quiet] [--bare] [--template=] [--shared[=]]\";\n+\"git init [-q | --quiet] [--bare] [--template=] [--shared[=]] [--restricted[=]]\";\n \n /*\n * If you want to, you can share the DB area with any number of branches.\n@@ -391,6 +400,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\tinit_shared_repository = PERM_GROUP;\n \t\telse if (!prefixcmp(arg, \"--shared=\"))\n \t\t\tinit_shared_repository = git_config_perm(\"arg\", arg+9);\n+\t\telse if (!strcmp(arg, \"--restricted\"))\n+\t\t\tinit_restricted_repository = PERM_MASK_USER;\n+\t\telse if (!prefixcmp(arg, \"--restricted=\"))\n+\t\t\tinit_restricted_repository =\n+\t\t\t\tgit_config_perm_mask(\"arg\", arg+13);\n \t\telse if (!strcmp(arg, \"-q\") || !strcmp(arg, \"--quiet\"))\n \t\t\tflags |= INIT_DB_QUIET;\n \t\telse\n-- \n1.6.2.1.473.g92672\n"},{"id":"109428","messageId":"200903252240.44049.johan@herland.net","threadId":"18516","inReplyTo":"200903252236.03010.johan@herland.net","subject":"[PATCH/RFC 5/7] Add tests for \"core.restrictedRepository\" and \"git init --restricted\"","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T21:40:43Z","receivedAt":"2009-03-25T21:40:43Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"These tests are based on - and analogous to - the existing tests for\n\"core.sharedRepository\" and \"git init --shared\"\n\nSigned-off-by: Johan Herland \n---\n t/t0001-init.sh | 24 +++++++-\n t/t1304-restricted-repo.sh | 132 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 153 insertions(+), 3 deletions(-)\n create mode 100755 t/t1304-restricted-repo.sh\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 5ac0a27..639a88d 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -167,7 +167,7 @@ test_expect_success 'init with --template (blank)' '\n \t! test -f template-blank/.git/info/exclude\n '\n \n-test_expect_success 'init --bare/--shared overrides system/global config' '\n+test_expect_success 'init --bare/--shared/--restricted overrides system/global config' '\n \t(\n \t\tHOME=\"`pwd`\" &&\n \t\texport HOME &&\n@@ -175,13 +175,16 @@ test_expect_success 'init --bare/--shared overrides system/global config' '\n \t\tunset GIT_CONFIG_NOGLOBAL &&\n \t\tgit config -f \"$test_config\" core.bare false &&\n \t\tgit config -f \"$test_config\" core.sharedRepository 0640 &&\n+\t\tgit config -f \"$test_config\" core.restrictedRepository 0027 &&\n \t\tmkdir init-bare-shared-override &&\n \t\tcd init-bare-shared-override &&\n-\t\tgit init --bare --shared=0666\n+\t\tgit init --bare --shared=0644 --restricted=0022\n \t) &&\n \tcheck_config init-bare-shared-override true unset &&\n-\ttest x0666 = \\\n+\ttest x0644 = \\\n \tx`git config -f init-bare-shared-override/config core.sharedRepository`\n+\ttest x0022 = \\\n+\tx`git config -f init-bare-shared-override/config core.restrictedRepository`\n '\n \n test_expect_success 'init honors global core.sharedRepository' '\n@@ -199,4 +202,19 @@ test_expect_success 'init honors global core.sharedRepository' '\n \tx`git config -f shared-honor-global/.git/config core.sharedRepository`\n '\n \n+test_expect_success 'init honors global core.restrictedRepository' '\n+\t(\n+\t\tHOME=\"`pwd`\" &&\n+\t\texport HOME &&\n+\t\ttest_config=\"$HOME\"/.gitconfig &&\n+\t\tunset GIT_CONFIG_NOGLOBAL &&\n+\t\tgit config -f \"$test_config\" core.restrictedRepository 0077 &&\n+\t\tmkdir restricted-honor-global &&\n+\t\tcd restricted-honor-global &&\n+\t\tgit init\n+\t) &&\n+\ttest x0077 = \\\n+\tx`git config -f restricted-honor-global/.git/config core.restrictedRepository`\n+'\n+\n test_done\ndiff --git a/t/t1304-restricted-repo.sh b/t/t1304-restricted-repo.sh\nnew file mode 100755\nindex 0000000..012cdf1\n--- /dev/null\n+++ b/t/t1304-restricted-repo.sh\n@@ -0,0 +1,132 @@\n+#!/bin/sh\n+#\n+# Copied and modified from t1301-shared-repo.sh\n+#\n+\n+test_description='Test restricted repository initialization'\n+\n+. ./test-lib.sh\n+\n+# Remove a default ACL from the test dir if possible.\n+setfacl -k . 2>/dev/null\n+\n+# User must have r/w permissions to the repo -> failure on --restricted=0600\n+test_expect_success 'restricted = 0600 (faulty permission u-rw)' '\n+\tmkdir sub && (\n+\t\tcd sub && git init --restricted=0600\n+\t)\n+\tret=\"$?\"\n+\trm -rf sub\n+\ttest $ret != \"0\"\n+'\n+\n+modebits () {\n+\tls -l \"$1\" | sed -e 's|^\\(..........\\).*|\\1|'\n+}\n+\n+for u in 0007 0077\n+do\n+\ttest_expect_success POSIXPERM \"restricted=group does not clear bits preset by umask $u\" '\n+\t\tmkdir sub && (\n+\t\t\tcd sub &&\n+\t\t\tumask $u &&\n+\t\t\tgit init --restricted=group &&\n+\t\t\ttest \"0007\" = \"$(git config core.restrictedrepository)\"\n+\t\t) &&\n+\t\tactual=$(ls -l sub/.git/HEAD)\n+\t\tcase \"$u$actual\" in\n+\t\t0007-rw-rw----*)\n+\t\t\t: happy\n+\t\t\t;;\n+\t\t0077-rw-------*)\n+\t\t\t: happy\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo Oops, .git/HEAD is not 06x0 but $actual\n+\t\t\tfalse\n+\t\t\t;;\n+\t\tesac\n+\t'\n+\trm -rf sub\n+done\n+\n+test_expect_success 'restricted=user' '\n+\tmkdir sub &&\n+\tcd sub &&\n+\tgit init --restricted=user &&\n+\ttest \"0077\" = \"$(git config core.restrictedrepository)\"\n+'\n+\n+test_expect_success POSIXPERM 'update-server-info honors core.restrictedRepository' '\n+\t: > a1 &&\n+\tgit add a1 &&\n+\ttest_tick &&\n+\tgit commit -m a1 &&\n+\tumask 0277 &&\n+\tgit update-server-info &&\n+\tactual=\"$(ls -l .git/info/refs)\" &&\n+\tcase \"$actual\" in\n+\t-r--------*)\n+\t\t: happy\n+\t\t;;\n+\t*)\n+\t\techo Oops, .git/info/refs is not 0400\n+\t\tfalse\n+\t\t;;\n+\tesac\n+'\n+\n+for u in\t0000:rw-rw-rw- \\\n+\t\t0002:rw-rw-r-- \\\n+\t\t0007:rw-rw---- \\\n+\t\t0027:rw-r----- \\\n+\t\t0077:rw-------\n+do\n+\tx=$(expr \"$u\" : \".*:\\([rw-]*\\)\") &&\n+\ty=$(echo \"$x\" | sed -e \"s/w/-/g\") &&\n+\tu=$(expr \"$u\" : \"\\([0-7]*\\)\"); test $? -le 1 &&\n+\tgit config core.restrictedrepository \"$u\" &&\n+\tumask 0222 &&\n+\ttest_expect_success POSIXPERM \"shared = $u ($y) ro\" '\n+\n+\t\trm -f .git/info/refs &&\n+\t\tgit update-server-info &&\n+\t\tactual=\"$(modebits .git/info/refs)\" &&\n+\t\ttest \"x$actual\" = \"x-$y\" || {\n+\t\t\tls -lt .git/info\n+\t\t\tfalse\n+\t\t}\n+\t'\n+\n+\tumask 0000 &&\n+\ttest_expect_success POSIXPERM \"shared = $u ($x) rw\" '\n+\n+\t\trm -f .git/info/refs &&\n+\t\tgit update-server-info &&\n+\t\tactual=\"$(modebits .git/info/refs)\" &&\n+\t\ttest \"x$actual\" = \"x-$x\" || {\n+\t\t\tls -lt .git/info\n+\t\t\tfalse\n+\t\t}\n+\n+\t'\n+\n+done\n+\n+test_expect_success POSIXPERM 'git reflog expire honors core.restrictedRepository' '\n+\tumask 0000\n+\tgit config core.restrictedRepository group &&\n+\tgit reflog expire --all &&\n+\tactual=\"$(ls -l .git/logs/refs/heads/master)\" &&\n+\tcase \"$actual\" in\n+\t-rw-rw----*)\n+\t\t: happy\n+\t\t;;\n+\t*)\n+\t\techo Ooops, .git/logs/refs/heads/master is not 0660 [$actual]\n+\t\tfalse\n+\t\t;;\n+\tesac\n+'\n+\n+test_done\n-- \n1.6.2.1.473.g92672\n"},{"id":"109429","messageId":"200903252241.23302.johan@herland.net","threadId":"18516","inReplyTo":"200903252236.03010.johan@herland.net","subject":"[PATCH/RFC 6/7] git-init: Apply correct mode bits to template files in shared/restricted repo","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T21:41:23Z","receivedAt":"2009-03-25T21:41:23Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Template dirs/files are copied into the new repo before the config file is\nparsed. This causes them to be created with default permissions (i.e. from\numask). When \"--shared\" and/or \"--restricted\" is given, this patch teaches\ngit-init to initialize the global variables controlling permissions on\ncreated files (shared_repository and/or restricted_repository) _before_\nthe templates are copied. They are thus created with the appropriate\npermissions.\n\nSigned-off-by: Johan Herland \n---\n builtin-init-db.c | 9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex 8e7fa2d..fb88f65 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -187,6 +187,15 @@ static int create_default_files(const char *template_path)\n \tsafe_create_dir(git_path(\"refs/heads\"), 1);\n \tsafe_create_dir(git_path(\"refs/tags\"), 1);\n \n+\t/*\n+\t * Set up shared/restricted_repository before copying templates, so\n+\t * that they are created with appropriate permissions.\n+\t */\n+\tif (init_shared_repository != -1)\n+\t\tshared_repository = init_shared_repository;\n+\tif (init_restricted_repository != -1)\n+\t\trestricted_repository = init_restricted_repository;\n+\n \t/* First copy the templates -- we might have the default\n \t * config file there, in which case we would want to read\n \t * from it after installing.\n-- \n1.6.2.1.473.g92672\n"},{"id":"109430","messageId":"200903252242.02212.johan@herland.net","threadId":"18516","inReplyTo":"200903252236.03010.johan@herland.net","subject":"[PATCH/RFC 7/7] Apply restricted permissions to loose objects and pack files","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-25T21:42:02Z","receivedAt":"2009-03-25T21:42:02Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Loose objects and pack files are normally created with mode 0444, but in a\nrepository that uses core.restrictedRepository to restrict file modes, we\nfurther limit permissions on loose objects and pack files, according to the\nrestrictedRepository setting.\n\nSigned-off-by: Johan Herland \n---\n cache.h | 1 +\n fast-import.c | 4 ++--\n http-push.c | 2 +-\n http-walker.c | 2 +-\n index-pack.c | 4 ++--\n sha1_file.c | 2 +-\n 6 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4730f33..82e562b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -509,6 +509,7 @@ extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int shared_repository;\n extern int restricted_repository;\n+#define PERM_SHA1_FILE (0444 & ~restricted_repository)\n extern const char *apply_default_whitespace;\n extern int zlib_compression_level;\n extern int core_compression_level;\ndiff --git a/fast-import.c b/fast-import.c\nindex beeac0d..feafe6f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -902,8 +902,8 @@ static char *keep_pack(char *curr_index_name)\n \tstatic const char *keep_msg = \"fast-import\";\n \tint keep_fd;\n \n-\tchmod(pack_data->pack_name, 0444);\n-\tchmod(curr_index_name, 0444);\n+\tchmod(pack_data->pack_name, PERM_SHA1_FILE);\n+\tchmod(curr_index_name, PERM_SHA1_FILE);\n \n \tkeep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);\n \tif (keep_fd < 0)\ndiff --git a/http-push.c b/http-push.c\nindex 6ce5a1d..e33044f 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -748,7 +748,7 @@ static void finish_request(struct transfer_request *request)\n \t\t\taborted = 1;\n \t\t}\n \t} else if (request->state == RUN_FETCH_LOOSE) {\n-\t\tfchmod(request->local_fileno, 0444);\n+\t\tfchmod(request->local_fileno, PERM_SHA1_FILE);\n \t\tclose(request->local_fileno); request->local_fileno = -1;\n \n \t\tif (request->curl_result != CURLE_OK &&\ndiff --git a/http-walker.c b/http-walker.c\nindex 0dbad3c..a0dd5d2 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -231,7 +231,7 @@ static void finish_object_request(struct object_request *obj_req)\n {\n \tstruct stat st;\n \n-\tfchmod(obj_req->local, 0444);\n+\tfchmod(obj_req->local, PERM_SHA1_FILE);\n \tclose(obj_req->local); obj_req->local = -1;\n \n \tif (obj_req->http_code == 416) {\ndiff --git a/index-pack.c b/index-pack.c\nindex 7546822..c82e60a 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -825,7 +825,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\tdie(\"cannot store pack file\");\n \t}\n \tif (from_stdin)\n-\t\tchmod(final_pack_name, 0444);\n+\t\tchmod(final_pack_name, PERM_SHA1_FILE);\n \n \tif (final_index_name != curr_index_name) {\n \t\tif (!final_index_name) {\n@@ -836,7 +836,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\tif (move_temp_to_file(curr_index_name, final_index_name))\n \t\t\tdie(\"cannot store index file\");\n \t}\n-\tchmod(final_index_name, 0444);\n+\tchmod(final_index_name, PERM_SHA1_FILE);\n \n \tif (!from_stdin) {\n \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\ndiff --git a/sha1_file.c b/sha1_file.c\nindex a354f06..ad63fe1 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2272,7 +2272,7 @@ static void close_sha1_file(int fd)\n {\n \tif (fsync_object_files)\n \t\tfsync_or_die(fd, \"sha1 file\");\n-\tfchmod(fd, 0444);\n+\tfchmod(fd, PERM_SHA1_FILE);\n \tif (close(fd) != 0)\n \t\tdie(\"unable to write sha1 file\");\n }\n-- \n1.6.2.1.473.g92672\n"},{"id":"109442","messageId":"7vwsadw5pz.fsf@gitster.siamese.dyndns.org","threadId":"18516","inReplyTo":"7v63hybaqd.fsf@gitster.siamese.dyndns.org","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-25T23:19:36Z","receivedAt":"2009-03-25T23:19:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johan Herland writes:\n>\n>> On Wednesday 25 March 2009, Junio C Hamano wrote:\n>>> You might like to try a patch like this (untested).\n>>>\n>>> path.c | 17 +++++------------\n>>> 1 files changed, 5 insertions(+), 12 deletions(-)\n>>\n>> Thanks!\n>>\n>> This works much better :)\n>>\n>> However, there are still some questions/issues:\n>>\n>> - t1301-shared-repo.sh fails:\n>> Oops, .git/HEAD is not 0664 but -rw-rw---- [...]\n>> * FAIL 3: shared=1 does not clear bits preset by umask 022\n>> (I guess this is expected, as your patch changes the assumptions)\n>\n> I'd rather say the patch breaks people's expectations.\n\nHow about doing it like this, instead?\n\nThis fixes the behaviour of octal notation to how it is defined in the\ndocumentation, while keeping the traditional \"loosen only\" semantics\nintact for \"group\" and \"everybody\".\n\nThree main points of this patch are:\n\n - For an explicit octal notation, the internal shared_repository variable\n is set to a negative value, so that we can tell \"group\" (which is to\n \"OR\" in 0660) and 0660 (which is to \"SET\" to 0660);\n\n - git-init did not set shared_repository variable early enough to affect\n the initial creation of many files, notably copied templates and the\n configuration. We set it very early when a command-line option\n specifies a custom value.\n\n - Loose object creation codepath first letsmkstemp() to create a new\n temporary file; depending on systems, this gets 0660 or 0600, which\n does not have anything to do with user's umask. To compensate for\n this, close_sha1_file() unconditionally doing fchmod(0444). For the\n traditional \"loosen-only\", adjust_shared_perm() call after it would be\n a no-op (the mode is already loose enough), but with the new behaviour\n it makes a difference.\n\nI think there are more places you could sprinkle adjust_shared_perm() for\npackfiles and other things. I didn't check, not because I wasn't\nuninterested, but because I am more interested in getting what\nadjust_shared_perm() itself does right.\n\n builtin-init-db.c | 12 ++++++++++--\n path.c | 33 ++++++++++++++++++---------------\n setup.c | 5 ++---\n sha1_file.c | 3 +++\n 4 files changed, 33 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex fc63d0f..4e02b33 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -194,6 +194,8 @@ static int create_default_files(const char *template_path)\n \n \tgit_config(git_default_config, NULL);\n \tis_bare_repository_cfg = init_is_bare_repository;\n+\n+\t/* reading existing config may have overwrote it */\n \tif (init_shared_repository != -1)\n \t\tshared_repository = init_shared_repository;\n \n@@ -312,12 +314,15 @@ int init_db(const char *template_dir, unsigned int flags)\n \t\t * and compatibility values for PERM_GROUP and\n \t\t * PERM_EVERYBODY.\n \t\t */\n-\t\tif (shared_repository == PERM_GROUP)\n+\t\tif (shared_repository < 0)\n+\t\t\t/* force to the mode value */\n+\t\t\tsprintf(buf, \"0%o\", -shared_repository);\n+\t\telse if (shared_repository == PERM_GROUP)\n \t\t\tsprintf(buf, \"%d\", OLD_PERM_GROUP);\n \t\telse if (shared_repository == PERM_EVERYBODY)\n \t\t\tsprintf(buf, \"%d\", OLD_PERM_EVERYBODY);\n \t\telse\n-\t\t\tsprintf(buf, \"0%o\", shared_repository);\n+\t\t\tdie(\"oops\");\n \t\tgit_config_set(\"core.sharedrepository\", buf);\n \t\tgit_config_set(\"receive.denyNonFastforwards\", \"true\");\n \t}\n@@ -397,6 +402,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\tusage(init_db_usage);\n \t}\n \n+\tif (init_shared_repository != -1)\n+\t\tshared_repository = init_shared_repository;\n+\n \t/*\n \t * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR\n \t * without --bare. Catch the error early.\ndiff --git a/path.c b/path.c\nindex e332b50..9026b6e 100644\n--- a/path.c\n+++ b/path.c\n@@ -314,33 +314,36 @@ char *enter_repo(char *path, int strict)\n int adjust_shared_perm(const char *path)\n {\n \tstruct stat st;\n-\tint mode;\n+\tint mode, tweak, shared;\n \n \tif (!shared_repository)\n \t\treturn 0;\n \tif (lstat(path, &st) < 0)\n \t\treturn -1;\n \tmode = st.st_mode;\n-\n-\tif (shared_repository) {\n-\t\tint tweak = shared_repository;\n-\t\tif (!(mode & S_IWUSR))\n-\t\t\ttweak &= ~0222;\n+\tif (shared_repository < 0)\n+\t\tshared = -shared_repository;\n+\telse\n+\t\tshared = shared_repository;\n+\ttweak = shared;\n+\n+\tif (!(mode & S_IWUSR))\n+\t\ttweak &= ~0222;\n+\tif (shared_repository < 0)\n+\t\tmode = (mode & ~0777) | tweak;\n+\telse\n \t\tmode |= tweak;\n-\t} else {\n-\t\t/* Preserve old PERM_UMASK behaviour */\n-\t\tif (mode & S_IWUSR)\n-\t\t\tmode |= S_IWGRP;\n-\t}\n \n \tif (S_ISDIR(mode)) {\n-\t\tmode |= FORCE_DIR_SET_GID;\n-\n \t\t/* Copy read bits to execute bits */\n-\t\tmode |= (shared_repository & 0444) >> 2;\n+\t\tmode |= FORCE_DIR_SET_GID;\n+\t\tmode |= (shared & 0444) >> 2;\n \t}\n \n-\tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n+\tif (((shared_repository < 0\n+\t ? (st.st_mode & (FORCE_DIR_SET_GID | 0777))\n+\t : (st.st_mode & mode)) != mode) &&\n+\t chmod(path, mode) < 0)\n \t\treturn -2;\n \treturn 0;\n }\ndiff --git a/setup.c b/setup.c\nindex 6c2deda..a430238 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -434,7 +434,7 @@ int git_config_perm(const char *var, const char *value)\n \n \t/*\n \t * Treat values 0, 1 and 2 as compatibility cases, otherwise it is\n-\t * a chmod value.\n+\t * a chmod value to restrict to.\n \t */\n \tswitch (i) {\n \tcase PERM_UMASK: /* 0 */\n@@ -456,7 +455,7 @@ int git_config_perm(const char *var, const char *value)\n \t * Mask filemode value. Others can not get write permission.\n \t * x flags for directories are handled separately.\n \t */\n-\treturn i & 0666;\n+\treturn -(i & 0666);\n }\n \n int check_repository_format_version(const char *var, const char *value, void *cb)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 54972f9..b4c12c4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2393,6 +2393,9 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \t\t\t\ttmpfile, strerror(errno));\n \t}\n \n+\tif (adjust_shared_perm(tmpfile))\n+\t\tdie(\"unable to set permission to '%s'\", tmpfile);\n+\n \treturn move_temp_to_file(tmpfile, filename);\n }\n \n"},{"id":"109447","messageId":"200903260122.24770.johan@herland.net","threadId":"18516","inReplyTo":"7vwsadw5pz.fsf@gitster.siamese.dyndns.org","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-26T00:22:24Z","receivedAt":"2009-03-26T00:22:24Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 26 March 2009, Junio C Hamano wrote:\n> How about doing it like this, instead?\n\nLooks good, and is obviously much less intrusive than my attempt.\n\nThere's still one issue as compared to my series: Hook scripts in\n.git/hooks lose their executable bit when copied from template dir.\nYou probably need to do some kind of special x-bit handling, similar\nto what's already done for directories.\n\nOther than that:\nTested-by: Johan Herland \n\n> I think there are more places you could sprinkle adjust_shared_perm() for\n> packfiles and other things. I didn't check, not because I wasn't\n> uninterested, but because I am more interested in getting what\n> adjust_shared_perm() itself does right.\n\nHere is my #7 patch, re-rolled on top of your patch to call\nadjust_shared_perm():\n\n---\n fast-import.c | 2 ++\n http-push.c | 1 +\n http-walker.c | 1 +\n index-pack.c | 2 ++\n 4 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex beeac0d..d73ee71 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -903,7 +903,9 @@ static char *keep_pack(char *curr_index_name)\n int keep_fd;\n \n chmod(pack_data->pack_name, 0444);\n+ adjust_shared_perm(pack_data->pack_name);\n chmod(curr_index_name, 0444);\n+ adjust_shared_perm(curr_index_name);\n \n keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);\n if (keep_fd < 0)\ndiff --git a/http-push.c b/http-push.c\nindex 6ce5a1d..a5acabf 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -750,6 +750,7 @@ static void finish_request(struct transfer_request *request)\n } else if (request->state == RUN_FETCH_LOOSE) {\n fchmod(request->local_fileno, 0444);\n close(request->local_fileno); request->local_fileno = -1;\n+ adjust_shared_perm(request->tmpfile);\n \n if (request->curl_result != CURLE_OK &&\n request->http_code != 416) {\ndiff --git a/http-walker.c b/http-walker.c\nindex 0dbad3c..24cfc45 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -233,6 +233,7 @@ static void finish_object_request(struct object_request *obj_req)\n \n fchmod(obj_req->local, 0444);\n close(obj_req->local); obj_req->local = -1;\n+ adjust_shared_perm(obj_req->tmpfile);\n \n if (obj_req->http_code == 416) {\n fprintf(stderr, \"Warning: requested range invalid; we may already have all the data.\\n\");\ndiff --git a/index-pack.c b/index-pack.c\nindex 7546822..7abe3f0 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -826,6 +826,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n }\n if (from_stdin)\n chmod(final_pack_name, 0444);\n+ adjust_shared_perm(final_pack_name);\n \n if (final_index_name != curr_index_name) {\n if (!final_index_name) {\n@@ -837,6 +838,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n die(\"cannot store index file\");\n }\n chmod(final_index_name, 0444);\n+ adjust_shared_perm(final_index_name);\n \n if (!from_stdin) {\n printf(\"%s\\n\", sha1_to_hex(sha1));\n-- \n1.6.2.1.473.g92672\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109498","messageId":"7vprg4rbmp.fsf@gitster.siamese.dyndns.org","threadId":"18516","inReplyTo":"200903260122.24770.johan@herland.net","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-26T07:23:10Z","receivedAt":"2009-03-26T07:23:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Thursday 26 March 2009, Junio C Hamano wrote:\n>> How about doing it like this, instead?\n>\n> Looks good, and is obviously much less intrusive than my attempt.\n>\n> There's still one issue as compared to my series: Hook scripts in\n> .git/hooks lose their executable bit when copied from template dir.\n> You probably need to do some kind of special x-bit handling, similar\n> to what's already done for directories.\n>\n> Other than that:\n> Tested-by: Johan Herland \n\nSorry to invalidate your Tested-by, but here is a re-roll using a slightly\ndifferent strategy.\n\nTo fix the loose object codepath, the earlier patch added a call to\nadjust_shared_perm() to write_loose_object() function, but after looking\nat your 7th patch, I realized that the pattern of file creation inside\n$GIT_DIR typically is to first create a temporary file, write to it, and\nthen finish it off by calling move_temp_to_file(). The true purpose of\nthe function is to \"finalize the file being created\", and it is misnamed\nin that it describes how its implementation does it currently (i.e. by\nrenaming the temporary file to its final name), but it makes perfect sense\nto call adjust_shared_perm() inside it as a part of finalization. I think\nthis should cover the codepaths your 7th patch fixed without actually\ntouching them.\n\nCould you eyeball and re-test it?\n\n-- >8 --\n[PATCH] \"core.sharedrepository = 0mode\" should set, not loosen\n\nThis fixes the behaviour of octal notation to how it is defined in the\ndocumentation, while keeping the traditional \"loosen only\" semantics\nintact for \"group\" and \"everybody\".\n\nThree main points of this patch are:\n\n - For an explicit octal notation, the internal shared_repository variable\n is set to a negative value, so that we can tell \"group\" (which is to\n \"OR\" in 0660) and 0660 (which is to \"SET\" to 0660);\n\n - git-init did not set shared_repository variable early enough to affect\n the initial creation of many files, notably copied templates and the\n configuration. We set it very early when a command-line option\n specifies a custom value.\n\n - Many codepaths create files inside $GIT_DIR by various codepaths that\n involve mkstemp(), and then call move_temp_to_file() to rename it to\n its final destination. We can add adjust_shared_perm() call here; for\n the traditional \"loosen-only\", this would be a no-op for many codepaths\n because the mode is already loose enough, but with the new behaviour it\n makes a difference.\n\nSigned-off-by: Junio C Hamano \n---\n builtin-init-db.c | 12 ++++++++++--\n path.c | 36 +++++++++++++++++++++---------------\n setup.c | 4 ++--\n sha1_file.c | 8 +++++++-\n t/t1301-shared-repo.sh | 37 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 77 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex ee3911f..8199e5d 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -195,6 +195,8 @@ static int create_default_files(const char *template_path)\n \n \tgit_config(git_default_config, NULL);\n \tis_bare_repository_cfg = init_is_bare_repository;\n+\n+\t/* reading existing config may have overwrote it */\n \tif (init_shared_repository != -1)\n \t\tshared_repository = init_shared_repository;\n \n@@ -313,12 +315,15 @@ int init_db(const char *template_dir, unsigned int flags)\n \t\t * and compatibility values for PERM_GROUP and\n \t\t * PERM_EVERYBODY.\n \t\t */\n-\t\tif (shared_repository == PERM_GROUP)\n+\t\tif (shared_repository < 0)\n+\t\t\t/* force to the mode value */\n+\t\t\tsprintf(buf, \"0%o\", -shared_repository);\n+\t\telse if (shared_repository == PERM_GROUP)\n \t\t\tsprintf(buf, \"%d\", OLD_PERM_GROUP);\n \t\telse if (shared_repository == PERM_EVERYBODY)\n \t\t\tsprintf(buf, \"%d\", OLD_PERM_EVERYBODY);\n \t\telse\n-\t\t\tsprintf(buf, \"0%o\", shared_repository);\n+\t\t\tdie(\"oops\");\n \t\tgit_config_set(\"core.sharedrepository\", buf);\n \t\tgit_config_set(\"receive.denyNonFastforwards\", \"true\");\n \t}\n@@ -398,6 +403,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\tusage(init_db_usage);\n \t}\n \n+\tif (init_shared_repository != -1)\n+\t\tshared_repository = init_shared_repository;\n+\n \t/*\n \t * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR\n \t * without --bare. Catch the error early.\ndiff --git a/path.c b/path.c\nindex e332b50..42898e0 100644\n--- a/path.c\n+++ b/path.c\n@@ -314,33 +314,39 @@ char *enter_repo(char *path, int strict)\n int adjust_shared_perm(const char *path)\n {\n \tstruct stat st;\n-\tint mode;\n+\tint mode, tweak, shared;\n \n \tif (!shared_repository)\n \t\treturn 0;\n \tif (lstat(path, &st) < 0)\n \t\treturn -1;\n \tmode = st.st_mode;\n-\n-\tif (shared_repository) {\n-\t\tint tweak = shared_repository;\n-\t\tif (!(mode & S_IWUSR))\n-\t\t\ttweak &= ~0222;\n+\tif (shared_repository < 0)\n+\t\tshared = -shared_repository;\n+\telse\n+\t\tshared = shared_repository;\n+\ttweak = shared;\n+\n+\tif (!(mode & S_IWUSR))\n+\t\ttweak &= ~0222;\n+\tif (mode & S_IXUSR)\n+\t\t/* Copy read bits to execute bits */\n+\t\ttweak |= (tweak & 0444) >> 2;\n+\tif (shared_repository < 0)\n+\t\tmode = (mode & ~0777) | tweak;\n+\telse\n \t\tmode |= tweak;\n-\t} else {\n-\t\t/* Preserve old PERM_UMASK behaviour */\n-\t\tif (mode & S_IWUSR)\n-\t\t\tmode |= S_IWGRP;\n-\t}\n \n \tif (S_ISDIR(mode)) {\n-\t\tmode |= FORCE_DIR_SET_GID;\n-\n \t\t/* Copy read bits to execute bits */\n-\t\tmode |= (shared_repository & 0444) >> 2;\n+\t\tmode |= (shared & 0444) >> 2;\n+\t\tmode |= FORCE_DIR_SET_GID;\n \t}\n \n-\tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n+\tif (((shared_repository < 0\n+\t ? (st.st_mode & (FORCE_DIR_SET_GID | 0777))\n+\t : (st.st_mode & mode)) != mode) &&\n+\t chmod(path, mode) < 0)\n \t\treturn -2;\n \treturn 0;\n }\ndiff --git a/setup.c b/setup.c\nindex 6c2deda..ebd60de 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -434,7 +434,7 @@ int git_config_perm(const char *var, const char *value)\n \n \t/*\n \t * Treat values 0, 1 and 2 as compatibility cases, otherwise it is\n-\t * a chmod value.\n+\t * a chmod value to restrict to.\n \t */\n \tswitch (i) {\n \tcase PERM_UMASK: /* 0 */\n@@ -456,7 +456,7 @@ int git_config_perm(const char *var, const char *value)\n \t * Mask filemode value. Others can not get write permission.\n \t * x flags for directories are handled separately.\n \t */\n-\treturn i & 0666;\n+\treturn -(i & 0666);\n }\n \n int check_repository_format_version(const char *var, const char *value, void *cb)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex a07aa4e..45987bd 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2243,11 +2243,15 @@ static void write_sha1_file_prepare(const void *buf, unsigned long len,\n }\n \n /*\n- * Move the just written object into its final resting place\n+ * Move the just written object into its final resting place.\n+ * NEEDSWORK: this should be renamed to finalize_temp_file() as\n+ * \"moving\" is only a part of what it does, when no patch between\n+ * master to pu changes the call sites of this function.\n */\n int move_temp_to_file(const char *tmpfile, const char *filename)\n {\n \tint ret = 0;\n+\n \tif (link(tmpfile, filename))\n \t\tret = errno;\n \n@@ -2275,6 +2279,8 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t\t/* FIXME!!! Collision check here ? */\n \t}\n \n+\tif (adjust_shared_perm(filename))\n+\t\treturn error(\"unable to set permission to '%s'\", filename);\n \treturn 0;\n }\n \ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 653362b..d459854 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -126,4 +126,41 @@ test_expect_success 'git reflog expire honors core.sharedRepository' '\n \tesac\n '\n \n+test_expect_success 'forced modes' '\n+\tmkdir -p templates/hooks &&\n+\techo update-server-info >templates/hooks/post-update &&\n+\tchmod +x templates/hooks/post-update &&\n+\techo : >random-file &&\n+\tmkdir new &&\n+\t(\n+\t\tcd new &&\n+\t\tumask 002 &&\n+\t\tgit init --shared=0660 --template=../templates &&\n+\t\t>frotz &&\n+\t\tgit add frotz &&\n+\t\tgit commit -a -m initial &&\n+\t\tgit repack\n+\t) &&\n+\tfind new/.git -print |\n+\txargs ls -ld >actual &&\n+\n+\t# Everything must be unaccessible to others\n+\ttest -z \"$(sed -n -e \"/^.......---/d\" actual)\" &&\n+\n+\t# All directories must have 2770\n+\ttest -z \"$(sed -n -e \"/^drwxrws---/d\" -e \"/^d/p\" actual)\" &&\n+\n+\t# post-update hook must be 0770\n+\ttest -z \"$(sed -n -e \"/post-update/{\n+\t\t/^-rwxrwx---/d\n+\t\tp\n+\t}\" actual)\" &&\n+\n+\t# All files inside objects must be 0440\n+\ttest -z \"$(sed -n -e \"/objects\\//{\n+\t\t/^d/d\n+\t\t/^-r--r-----/d\n+\t}\" actual)\"\n+'\n+\n test_done\n-- \n1.6.2.1.394.g50949\n"},{"id":"109502","messageId":"200903260929.58321.johan@herland.net","threadId":"18516","inReplyTo":"7vprg4rbmp.fsf@gitster.siamese.dyndns.org","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-26T08:29:58Z","receivedAt":"2009-03-26T08:29:58Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 26 March 2009, Junio C Hamano wrote:\n> To fix the loose object codepath, the earlier patch added a call to\n> adjust_shared_perm() to write_loose_object() function, but after looking\n> at your 7th patch, I realized that the pattern of file creation inside\n> $GIT_DIR typically is to first create a temporary file, write to it, and\n> then finish it off by calling move_temp_to_file(). The true purpose of\n> the function is to \"finalize the file being created\", and it is misnamed\n> in that it describes how its implementation does it currently (i.e. by\n> renaming the temporary file to its final name), but it makes perfect\n> sense to call adjust_shared_perm() inside it as a part of finalization. \n> I think this should cover the codepaths your 7th patch fixed without\n> actually touching them.\n\nYes, with one exception:\n\nFor the two cases index-pack.c, the chmod(foo, 0444) happens AFTER the\ncorresponding call to move_temp_to_file(xyzzy, foo). The chmod() in\nadjust_shared_perms() would thus be overridden by the chmod(foo, 0444),\nwhich is not what we want. In both cases, I think the chmod(foo, 0444)\ncan safely be moved up above the call to move_temp_to_file(). Something\nlike this (although I'm not sure about the semantics of 'from_stdin'):\n\ndiff --git a/index-pack.c b/index-pack.c\nindex 7546822..d289b6a 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -815,6 +815,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t}\n \t}\n \n+\tif (from_stdin)\n+\t\tchmod(final_pack_name, 0444);\n \tif (final_pack_name != curr_pack_name) {\n \t\tif (!final_pack_name) {\n \t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n@@ -824,9 +826,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\tif (move_temp_to_file(curr_pack_name, final_pack_name))\n \t\t\tdie(\"cannot store pack file\");\n \t}\n-\tif (from_stdin)\n-\t\tchmod(final_pack_name, 0444);\n \n+\tchmod(final_index_name, 0444);\n \tif (final_index_name != curr_index_name) {\n \t\tif (!final_index_name) {\n \t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n@@ -836,7 +837,6 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\tif (move_temp_to_file(curr_index_name, final_index_name))\n \t\t\tdie(\"cannot store index file\");\n \t}\n-\tchmod(final_index_name, 0444);\n \n \tif (!from_stdin) {\n \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n\n\n> Could you eyeball and re-test it?\n\n> Signed-off-by: Junio C Hamano \n\nTested-by: Johan Herland \n\n> --- a/builtin-init-db.c\n> +++ b/builtin-init-db.c\n> @@ -195,6 +195,8 @@ static int create_default_files(const char\n> *template_path)\n>\n> \tgit_config(git_default_config, NULL);\n> \tis_bare_repository_cfg = init_is_bare_repository;\n> +\n> +\t/* reading existing config may have overwrote it */\n\ns/overwrote/overwritten/\n\nOtherwise OK, AFAICS.\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109503","messageId":"49CB3FA0.8030408@viscovery.net","threadId":"18516","inReplyTo":"200903260929.58321.johan@herland.net","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-26T08:41:04Z","receivedAt":"2009-03-26T08:41:04Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> For the two cases index-pack.c, the chmod(foo, 0444) happens AFTER the\n> corresponding call to move_temp_to_file(xyzzy, foo). The chmod() in\n> adjust_shared_perms() would thus be overridden by the chmod(foo, 0444),\n> which is not what we want. In both cases, I think the chmod(foo, 0444)\n> can safely be moved up above the call to move_temp_to_file(). Something\n> like this (although I'm not sure about the semantics of 'from_stdin'):\n> \n> diff --git a/index-pack.c b/index-pack.c\n> index 7546822..d289b6a 100644\n> --- a/index-pack.c\n> +++ b/index-pack.c\n> @@ -815,6 +815,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n> \t\t}\n> \t}\n> \n> +\tif (from_stdin)\n> +\t\tchmod(final_pack_name, 0444);\n> \tif (final_pack_name != curr_pack_name) {\n> \t\tif (!final_pack_name) {\n> \t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n> @@ -824,9 +826,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n> \t\tif (move_temp_to_file(curr_pack_name, final_pack_name))\n> \t\t\tdie(\"cannot store pack file\");\n> \t}\n> -\tif (from_stdin)\n> -\t\tchmod(final_pack_name, 0444);\n> \n> +\tchmod(final_index_name, 0444);\n> \tif (final_index_name != curr_index_name) {\n> \t\tif (!final_index_name) {\n> \t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n> @@ -836,7 +837,6 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n> \t\tif (move_temp_to_file(curr_index_name, final_index_name))\n> \t\t\tdie(\"cannot store index file\");\n> \t}\n> -\tchmod(final_index_name, 0444);\n> \n> \tif (!from_stdin) {\n> \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n\nYou certainly meant to use the curr_*_name variants in the chmod lines,\nno? This effectively undoes 33b65030, but that is not so good: On Windows\nwe cannot rename read-only files.\n\n-- Hannes\n"},{"id":"109511","messageId":"200903261044.58140.johan@herland.net","threadId":"18516","inReplyTo":"49CB3FA0.8030408@viscovery.net","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-26T09:44:57Z","receivedAt":"2009-03-26T09:44:57Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 26 March 2009, Johannes Sixt wrote:\n> You certainly meant to use the curr_*_name variants in the chmod lines,\n> no?\n\nOops. Yes. My bad.\n\n> This effectively undoes 33b65030, but that is not so good: On Windows\n> we cannot rename read-only files.\n\nWhat about this: Add a \"mode\" argument to move_temp_to_file(), used to\npass in the 0444, and then within move_temp_to_file() we can do the\nchmod(foo, 0444) _after_ renaming the file, but _before_ calling\nadjust_shared_perm().\n\nSomething like this on top of Junio's patch:\n\n----------------------->8-------------8<-------------------\ndiff --git a/cache.h b/cache.h\nindex 9cf5a13..9b28b98 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -652,7 +652,8 @@ extern int do_check_packed_object_crc;\n \n extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type);\n \n-extern int move_temp_to_file(const char *tmpfile, const char *filename);\n+/* Set mode to -1 if you don't want to explicitly change the file mode */\n+extern int finalize_temp_file(const char *tmpfile, const char *filename, int mode);\n \n extern int has_sha1_pack(const unsigned char *sha1);\n extern int has_sha1_file(const unsigned char *sha1);\ndiff --git a/fast-import.c b/fast-import.c\nindex beeac0d..a5556e9 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -902,9 +902,6 @@ static char *keep_pack(char *curr_index_name)\n \tstatic const char *keep_msg = \"fast-import\";\n \tint keep_fd;\n \n-\tchmod(pack_data->pack_name, 0444);\n-\tchmod(curr_index_name, 0444);\n-\n \tkeep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);\n \tif (keep_fd < 0)\n \t\tdie(\"cannot create keep file\");\n@@ -914,12 +911,12 @@ static char *keep_pack(char *curr_index_name)\n \n \tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n \t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n-\tif (move_temp_to_file(pack_data->pack_name, name))\n+\tif (finalize_temp_file(pack_data->pack_name, name, 0444))\n \t\tdie(\"cannot store pack file\");\n \n \tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n \t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n-\tif (move_temp_to_file(curr_index_name, name))\n+\tif (finalize_temp_file(curr_index_name, name, 0444))\n \t\tdie(\"cannot store index file\");\n \treturn name;\n }\ndiff --git a/http-push.c b/http-push.c\nindex 6ce5a1d..5be6600 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -748,7 +748,6 @@ static void finish_request(struct transfer_request *request)\n \t\t\taborted = 1;\n \t\t}\n \t} else if (request->state == RUN_FETCH_LOOSE) {\n-\t\tfchmod(request->local_fileno, 0444);\n \t\tclose(request->local_fileno); request->local_fileno = -1;\n \n \t\tif (request->curl_result != CURLE_OK &&\n@@ -769,9 +768,10 @@ static void finish_request(struct transfer_request *request)\n \t\t\t\tunlink(request->tmpfile);\n \t\t\t} else {\n \t\t\t\trequest->rename =\n-\t\t\t\t\tmove_temp_to_file(\n+\t\t\t\t\tfinalize_temp_file(\n \t\t\t\t\t\trequest->tmpfile,\n-\t\t\t\t\t\trequest->filename);\n+\t\t\t\t\t\trequest->filename,\n+\t\t\t\t\t\t0444);\n \t\t\t\tif (request->rename == 0) {\n \t\t\t\t\trequest->obj->flags |= (LOCAL | REMOTE);\n \t\t\t\t}\n@@ -794,8 +794,9 @@ static void finish_request(struct transfer_request *request)\n \n \t\t\tfclose(request->local_stream);\n \t\t\trequest->local_stream = NULL;\n-\t\t\tif (!move_temp_to_file(request->tmpfile,\n-\t\t\t\t\t request->filename)) {\n+\t\t\tif (!finalize_temp_file(request->tmpfile,\n+\t\t\t\t\trequest->filename,\n+\t\t\t\t\t-1)) {\n \t\t\t\ttarget = (struct packed_git *)request->userData;\n \t\t\t\ttarget->pack_size = pack_size;\n \t\t\t\tlst = &repo->packs;\n@@ -1007,7 +1008,7 @@ static int fetch_index(unsigned char *sha1)\n \tfree(url);\n \tfclose(indexfile);\n \n-\treturn move_temp_to_file(tmpfile, filename);\n+\treturn finalize_temp_file(tmpfile, filename, -1);\n }\n \n static int setup_index(unsigned char *sha1)\ndiff --git a/http-walker.c b/http-walker.c\nindex 0dbad3c..7da78e1 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -231,7 +231,6 @@ static void finish_object_request(struct object_request *obj_req)\n {\n \tstruct stat st;\n \n-\tfchmod(obj_req->local, 0444);\n \tclose(obj_req->local); obj_req->local = -1;\n \n \tif (obj_req->http_code == 416) {\n@@ -254,7 +253,7 @@ static void finish_object_request(struct object_request *obj_req)\n \t\treturn;\n \t}\n \tobj_req->rename =\n-\t\tmove_temp_to_file(obj_req->tmpfile, obj_req->filename);\n+\t\tfinalize_temp_file(obj_req->tmpfile, obj_req->filename, 0444);\n \n \tif (obj_req->rename == 0)\n \t\twalker_say(obj_req->walker, \"got %s\\n\", sha1_to_hex(obj_req->sha1));\n@@ -429,7 +428,7 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch\n \n \tfclose(indexfile);\n \n-\treturn move_temp_to_file(tmpfile, filename);\n+\treturn finalize_temp_file(tmpfile, filename, -1);\n }\n \n static int setup_index(struct walker *walker, struct alt_base *repo, unsigned char *sha1)\n@@ -788,7 +787,7 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha\n \ttarget->pack_size = ftell(packfile);\n \tfclose(packfile);\n \n-\tret = move_temp_to_file(tmpfile, filename);\n+\tret = finalize_temp_file(tmpfile, filename, -1);\n \tif (ret)\n \t\treturn ret;\n \ndiff --git a/index-pack.c b/index-pack.c\nindex 7546822..237acd1 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -821,10 +821,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n \t\t\tfinal_pack_name = name;\n \t\t}\n-\t\tif (move_temp_to_file(curr_pack_name, final_pack_name))\n+\t\tif (finalize_temp_file(curr_pack_name, final_pack_name, 0444))\n \t\t\tdie(\"cannot store pack file\");\n-\t}\n-\tif (from_stdin)\n+\t} else if (from_stdin)\n \t\tchmod(final_pack_name, 0444);\n \n \tif (final_index_name != curr_index_name) {\n@@ -833,10 +832,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n \t\t\tfinal_index_name = name;\n \t\t}\n-\t\tif (move_temp_to_file(curr_index_name, final_index_name))\n+\t\tif (finalize_temp_file(curr_index_name, final_index_name, 0444))\n \t\t\tdie(\"cannot store index file\");\n-\t}\n-\tchmod(final_index_name, 0444);\n+\t} else\n+\t\tchmod(final_index_name, 0444);\n \n \tif (!from_stdin) {\n \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 12e0dfd..3c611f4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2215,13 +2215,8 @@ static void write_sha1_file_prepare(const void *buf, unsigned long len,\n \tgit_SHA1_Final(sha1, &c);\n }\n \n-/*\n- * Move the just written object into its final resting place.\n- * NEEDSWORK: this should be renamed to finalize_temp_file() as\n- * \"moving\" is only a part of what it does, when no patch between\n- * master to pu changes the call sites of this function.\n- */\n-int move_temp_to_file(const char *tmpfile, const char *filename)\n+/* Move the just written object into its final resting place. */\n+int finalize_temp_file(const char *tmpfile, const char *filename, int mode)\n {\n \tint ret = 0;\n \n@@ -2252,8 +2247,10 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t\t/* FIXME!!! Collision check here ? */\n \t}\n \n+\tif (mode > 0 && chmod(filename, mode))\n+\t\treturn error(\"unable to set permission %04o on '%s'\", mode, filename);\n \tif (adjust_shared_perm(filename))\n-\t\treturn error(\"unable to set permission to '%s'\", filename);\n+\t\treturn error(\"unable to adjust permission on '%s'\", filename);\n \treturn 0;\n }\n \n@@ -2278,7 +2275,6 @@ static void close_sha1_file(int fd)\n {\n \tif (fsync_object_files)\n \t\tfsync_or_die(fd, \"sha1 file\");\n-\tfchmod(fd, 0444);\n \tif (close(fd) != 0)\n \t\tdie(\"error when closing sha1 file (%s)\", strerror(errno));\n }\n@@ -2386,7 +2382,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \t\t\t\ttmpfile, strerror(errno));\n \t}\n \n-\treturn move_temp_to_file(tmpfile, filename);\n+\treturn finalize_temp_file(tmpfile, filename, 0444);\n }\n \n int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n----------------------->8-------------8<-------------------\n\n\nIn the above patch, I've passed mode == -1 to finalize_temp_file() from all\ncallsites where there was no corresponding (f)chmod(foo, 0444). However,\nafter looking at the context (these are all either packs or loose objects),\nI'm wondering if we shouldn't pass mode == 0444 for all of these. At which\npoint we could replace the above patch with this much simpler version:\n\n----------------------->8-------------8<-------------------\ndiff --git a/fast-import.c b/fast-import.c\nindex beeac0d..1df3e86 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -902,9 +902,6 @@ static char *keep_pack(char *curr_index_name)\n \tstatic const char *keep_msg = \"fast-import\";\n \tint keep_fd;\n \n-\tchmod(pack_data->pack_name, 0444);\n-\tchmod(curr_index_name, 0444);\n-\n \tkeep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);\n \tif (keep_fd < 0)\n \t\tdie(\"cannot create keep file\");\ndiff --git a/http-push.c b/http-push.c\nindex 6ce5a1d..e465b20 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -748,7 +748,6 @@ static void finish_request(struct transfer_request *request)\n \t\t\taborted = 1;\n \t\t}\n \t} else if (request->state == RUN_FETCH_LOOSE) {\n-\t\tfchmod(request->local_fileno, 0444);\n \t\tclose(request->local_fileno); request->local_fileno = -1;\n \n \t\tif (request->curl_result != CURLE_OK &&\ndiff --git a/http-walker.c b/http-walker.c\nindex 0dbad3c..c5a3ea3 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -231,7 +231,6 @@ static void finish_object_request(struct object_request *obj_req)\n {\n \tstruct stat st;\n \n-\tfchmod(obj_req->local, 0444);\n \tclose(obj_req->local); obj_req->local = -1;\n \n \tif (obj_req->http_code == 416) {\ndiff --git a/index-pack.c b/index-pack.c\nindex 7546822..6e93ee6 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -823,8 +823,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t}\n \t\tif (move_temp_to_file(curr_pack_name, final_pack_name))\n \t\t\tdie(\"cannot store pack file\");\n-\t}\n-\tif (from_stdin)\n+\t} else if (from_stdin)\n \t\tchmod(final_pack_name, 0444);\n \n \tif (final_index_name != curr_index_name) {\n@@ -835,8 +834,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t}\n \t\tif (move_temp_to_file(curr_index_name, final_index_name))\n \t\t\tdie(\"cannot store index file\");\n-\t}\n-\tchmod(final_index_name, 0444);\n+\t} else\n+\t\tchmod(final_index_name, 0444);\n \n \tif (!from_stdin) {\n \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 12e0dfd..87ac53b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2252,7 +2252,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t\t/* FIXME!!! Collision check here ? */\n \t}\n \n-\tif (adjust_shared_perm(filename))\n+\tif (chmod(filename, 0444) || adjust_shared_perm(filename))\n \t\treturn error(\"unable to set permission to '%s'\", filename);\n \treturn 0;\n }\n@@ -2278,7 +2278,6 @@ static void close_sha1_file(int fd)\n {\n \tif (fsync_object_files)\n \t\tfsync_or_die(fd, \"sha1 file\");\n-\tfchmod(fd, 0444);\n \tif (close(fd) != 0)\n \t\tdie(\"error when closing sha1 file (%s)\", strerror(errno));\n }\n----------------------->8-------------8<-------------------\n\n\n(We could also add an optional \"mode\" argument to adjust_shared_perm(), to\nget rid of the double chmod().)\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109517","messageId":"49CB51E2.9010903@viscovery.net","threadId":"18516","inReplyTo":"200903261044.58140.johan@herland.net","subject":"Re: [BUG?] How to make a shared/restricted repo?","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-26T09:58:58Z","receivedAt":"2009-03-26T09:58:58Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johan Herland schrieb:\n> In the above patch, I've passed mode == -1 to finalize_temp_file() from all\n> callsites where there was no corresponding (f)chmod(foo, 0444). However,\n> after looking at the context (these are all either packs or loose objects),\n> I'm wondering if we shouldn't pass mode == 0444 for all of these. At which\n> point we could replace the above patch with this much simpler version:\n\nIndeed!\n\n> (We could also add an optional \"mode\" argument to adjust_shared_perm(), to\n> get rid of the double chmod().)\n\nAnd I think you should do that, otherwise you have a short time window\nwhere the permissions of a pack or loose object is less restrictive than\nyou want.\n\n-- Hannes\n"},{"id":"109539","messageId":"200903261602.37857.johan@herland.net","threadId":"18516","inReplyTo":"49CB51E2.9010903@viscovery.net","subject":"[PATCH 0/2] chmod cleanup (Was: [BUG?] How to make a shared/restricted repo?)","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-26T15:02:37Z","receivedAt":"2009-03-26T15:02:37Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Thursday 26 March 2009, Johannes Sixt wrote:\n> Johan Herland schrieb:\n> > In the above patch, I've passed mode == -1 to finalize_temp_file()\n> > from all callsites where there was no corresponding (f)chmod(foo,\n> > 0444). However, after looking at the context (these are all either\n> > packs or loose objects), I'm wondering if we shouldn't pass mode ==\n> > 0444 for all of these. At which point we could replace the above\n> > patch with this much simpler version:\n>\n> Indeed!\n>\n> > (We could also add an optional \"mode\" argument to\n> > adjust_shared_perm(), to get rid of the double chmod().)\n>\n> And I think you should do that, otherwise you have a short time\n> window where the permissions of a pack or loose object is less\n> restrictive than you want.\n\nOk, here's a cleaned-up series on top of Junio's patch. It should resolve\nthe chmod()-after-adjust_shared_perm() issue in Junio's patch, as well as\nthe rename-after-chmod Windows issue reported by Hannes.\n\nThe first patch is the second alternative I sent in an earlier mail.\nThe second patch resolves the double chmod() left by the first patch.\n\n\nJohan Herland (2):\n Move chmod(foo, 0444) into move_temp_to_file()\n Resolve double chmod() in move_temp_to_file()\n\n cache.h | 1 +\n fast-import.c | 3 ---\n http-push.c | 1 -\n http-walker.c | 1 -\n index-pack.c | 7 +++----\n path.c | 26 +++++++++++++++++++-------\n sha1_file.c | 3 +--\n 7 files changed, 24 insertions(+), 18 deletions(-)\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109540","messageId":"200903261616.47185.johan@herland.net","threadId":"18516","inReplyTo":"200903261602.37857.johan@herland.net","subject":"[PATCH 1/2] Move chmod(foo, 0444) into move_temp_to_file()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-26T15:16:47Z","receivedAt":"2009-03-26T15:16:47Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"When writing out a loose object or a pack (index), move_temp_to_file() is\ncalled to finalize the resulting file. These files (loose files and packs)\nshould all have permission mode 0444 (modulo adjust_shared_perm()).\nTherefore, instead of doing chmod(foo, 0444) explicitly from each callsite\n(or even forgetting to chmod() at all), do the chmod() call from within\nmove_temp_to_file().\n\nSigned-off-by: Johan Herland \n---\n fast-import.c | 3 ---\n http-push.c | 1 -\n http-walker.c | 1 -\n index-pack.c | 7 +++----\n sha1_file.c | 3 +--\n 5 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex db44da3..23c496d 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -903,9 +903,6 @@ static char *keep_pack(char *curr_index_name)\n \tstatic const char *keep_msg = \"fast-import\";\n \tint keep_fd;\n \n-\tchmod(pack_data->pack_name, 0444);\n-\tchmod(curr_index_name, 0444);\n-\n \tkeep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);\n \tif (keep_fd < 0)\n \t\tdie(\"cannot create keep file\");\ndiff --git a/http-push.c b/http-push.c\nindex 6ce5a1d..e465b20 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -748,7 +748,6 @@ static void finish_request(struct transfer_request *request)\n \t\t\taborted = 1;\n \t\t}\n \t} else if (request->state == RUN_FETCH_LOOSE) {\n-\t\tfchmod(request->local_fileno, 0444);\n \t\tclose(request->local_fileno); request->local_fileno = -1;\n \n \t\tif (request->curl_result != CURLE_OK &&\ndiff --git a/http-walker.c b/http-walker.c\nindex 0dbad3c..c5a3ea3 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -231,7 +231,6 @@ static void finish_object_request(struct object_request *obj_req)\n {\n \tstruct stat st;\n \n-\tfchmod(obj_req->local, 0444);\n \tclose(obj_req->local); obj_req->local = -1;\n \n \tif (obj_req->http_code == 416) {\ndiff --git a/index-pack.c b/index-pack.c\nindex 7546822..6e93ee6 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -823,8 +823,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t}\n \t\tif (move_temp_to_file(curr_pack_name, final_pack_name))\n \t\t\tdie(\"cannot store pack file\");\n-\t}\n-\tif (from_stdin)\n+\t} else if (from_stdin)\n \t\tchmod(final_pack_name, 0444);\n \n \tif (final_index_name != curr_index_name) {\n@@ -835,8 +834,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t}\n \t\tif (move_temp_to_file(curr_index_name, final_index_name))\n \t\t\tdie(\"cannot store index file\");\n-\t}\n-\tchmod(final_index_name, 0444);\n+\t} else\n+\t\tchmod(final_index_name, 0444);\n \n \tif (!from_stdin) {\n \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 12e0dfd..87ac53b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2252,7 +2252,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t\t/* FIXME!!! Collision check here ? */\n \t}\n \n-\tif (adjust_shared_perm(filename))\n+\tif (chmod(filename, 0444) || adjust_shared_perm(filename))\n \t\treturn error(\"unable to set permission to '%s'\", filename);\n \treturn 0;\n }\n@@ -2278,7 +2278,6 @@ static void close_sha1_file(int fd)\n {\n \tif (fsync_object_files)\n \t\tfsync_or_die(fd, \"sha1 file\");\n-\tfchmod(fd, 0444);\n \tif (close(fd) != 0)\n \t\tdie(\"error when closing sha1 file (%s)\", strerror(errno));\n }\n-- \n1.6.1.2.461.g5bad6\n"},{"id":"109541","messageId":"200903261617.48362.johan@herland.net","threadId":"18516","inReplyTo":"200903261602.37857.johan@herland.net","subject":"[PATCH 2/2] Resolve double chmod() in move_temp_to_file()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-26T15:17:48Z","receivedAt":"2009-03-26T15:17:48Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"move_temp_to_file() used to chmod(foo, 0444) immediately before calling\nadjust_shared_perm() which potentially does another call to chmod().\n\nThis patch splits a new function (called get_shared_perm()) out of\nadjust_shared_perm(). The new function adjusts a given file mode value\naccording to the shared_repository setting, and returns the resulting\nmode. It is used in move_temp_file() to generate the correct and final\npermissions to pass to chmod() in a single call.\n\nSigned-off-by: Johan Herland \n---\n cache.h | 1 +\n path.c | 26 +++++++++++++++++++-------\n sha1_file.c | 2 +-\n 3 files changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 9cf5a13..b24eb3a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -623,6 +623,7 @@ enum sharedrepo {\n \tPERM_EVERYBODY = 0664,\n };\n int git_config_perm(const char *var, const char *value);\n+int get_shared_perm(int mode);\n int adjust_shared_perm(const char *path);\n int safe_create_leading_directories(char *path);\n int safe_create_leading_directories_const(const char *path);\ndiff --git a/path.c b/path.c\nindex 42898e0..497db19 100644\n--- a/path.c\n+++ b/path.c\n@@ -311,16 +311,13 @@ char *enter_repo(char *path, int strict)\n \treturn NULL;\n }\n \n-int adjust_shared_perm(const char *path)\n+int get_shared_perm(int mode)\n {\n-\tstruct stat st;\n-\tint mode, tweak, shared;\n+\tint tweak, shared;\n \n \tif (!shared_repository)\n-\t\treturn 0;\n-\tif (lstat(path, &st) < 0)\n-\t\treturn -1;\n-\tmode = st.st_mode;\n+\t\treturn mode;\n+\n \tif (shared_repository < 0)\n \t\tshared = -shared_repository;\n \telse\n@@ -343,6 +340,21 @@ int adjust_shared_perm(const char *path)\n \t\tmode |= FORCE_DIR_SET_GID;\n \t}\n \n+\treturn mode;\n+}\n+\n+int adjust_shared_perm(const char *path)\n+{\n+\tstruct stat st;\n+\tint mode, tweak, shared;\n+\n+\tif (!shared_repository)\n+\t\treturn 0;\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\n+\tmode = get_shared_perm(st.st_mode);\n+\n \tif (((shared_repository < 0\n \t ? (st.st_mode & (FORCE_DIR_SET_GID | 0777))\n \t : (st.st_mode & mode)) != mode) &&\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 87ac53b..05af3c5 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2252,7 +2252,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t\t/* FIXME!!! Collision check here ? */\n \t}\n \n-\tif (chmod(filename, 0444) || adjust_shared_perm(filename))\n+\tif (chmod(filename, get_shared_perm(0444)))\n \t\treturn error(\"unable to set permission to '%s'\", filename);\n \treturn 0;\n }\n-- \n1.6.1.2.461.g5bad6\n"},{"id":"109688","messageId":"7vskky18ds.fsf@gitster.siamese.dyndns.org","threadId":"18516","inReplyTo":"200903261616.47185.johan@herland.net","subject":"Re: [PATCH 1/2] Move chmod(foo, 0444) into move_temp_to_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-28T06:14:39Z","receivedAt":"2009-03-28T06:14:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> When writing out a loose object or a pack (index), move_temp_to_file() is\n> called to finalize the resulting file. These files (loose files and packs)\n> should all have permission mode 0444 (modulo adjust_shared_perm()).\n> Therefore, instead of doing chmod(foo, 0444) explicitly from each callsite\n> (or even forgetting to chmod() at all), do the chmod() call from within\n> move_temp_to_file().\n>\n> Signed-off-by: Johan Herland \n> ---\n\nI think you would need this on top.\n\n-- >8 --\nmove_temp_to_file(): do not forget to chmod() in \"Coda hack\" codepath\n\nNow move_temp_to_file() is responsible for doing everything that is\nnecessary to turn a tempfile in $GIT_DIR into its final form, it must make\nsure \"Coda hack\" codepath correctly makes the file read-only.\n\nSigned-off-by: Junio C Hamano \n---\n sha1_file.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3bd20e7..8869488 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2268,7 +2268,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t */\n \tif (ret && ret != EEXIST) {\n \t\tif (!rename(tmpfile, filename))\n-\t\t\treturn 0;\n+\t\t\tgoto out;\n \t\tret = errno;\n \t}\n \tunlink(tmpfile);\n@@ -2279,6 +2279,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t\t/* FIXME!!! Collision check here ? */\n \t}\n \n+out:\n \tif (chmod(filename, 0444) || adjust_shared_perm(filename))\n \t\treturn error(\"unable to set permission to '%s'\", filename);\n \treturn 0;\n"},{"id":"109689","messageId":"7viqlu1837.fsf@gitster.siamese.dyndns.org","threadId":"18516","inReplyTo":"200903261617.48362.johan@herland.net","subject":"Re: [PATCH 2/2] Resolve double chmod() in move_temp_to_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-28T06:21:00Z","receivedAt":"2009-03-28T06:21:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 87ac53b..05af3c5 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2252,7 +2252,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n> \t\t/* FIXME!!! Collision check here ? */\n> \t}\n> \n> -\tif (chmod(filename, 0444) || adjust_shared_perm(filename))\n> +\tif (chmod(filename, get_shared_perm(0444)))\n\nYour get_shared_perm() will end up feeding 0444 to S_ISDIR(), which would\nmost likely say \"no\" and cause real harm, but there is no guarantee that\nwe won't start checking S_ISREG() or other things in get_shared_perm()\nlater. I do not like this.\n\nHow about doing it this way instead?\n\nOne thing to note is that we seem to have been passing what we read from\nst.st_mode, together with S_IFMT bits, to chmod(2); I do not think I've\nseen any breakage reports on exotic systems (glibc on Linux seems to\nignore the higher bits), but from my reading of POSIX, I would not be\nsurprised if somebody's chmod(2) returned EINVAL.\n\n-- >8 --\nset_shared_perm(): sometimes we know what the final mode bits should look like\n\nadjust_shared_perm() first obtains the mode bits from lstat(2), expecting\nto find what the result of applying user's umask is, and then tweaked it\nas necessary. When the file to be adjusted is created with mkstemp(3),\nhowever, the mode thusly obtained does not have anything to do with usre's\numask, and we would need to start from 0444 in such a case and there is no\npoint running lstat(2) for such a path.\n\nThis introduces a new API set_shared_perm() to bypass the lstat(2) and\ninstead force setting the mode bits to the desired value directly.\nadjust_shared_perm() becomes a thin wrapper to the function.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 3 ++-\n path.c | 25 ++++++++++++++++---------\n sha1_file.c | 4 ++--\n 3 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 189151d..e283bbe 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -613,7 +613,8 @@ enum sharedrepo {\n \tPERM_EVERYBODY = 0664,\n };\n int git_config_perm(const char *var, const char *value);\n-int adjust_shared_perm(const char *path);\n+int set_shared_perm(const char *path, int mode);\n+#define adjust_shared_perm(path) set_shared_perm((path), 0)\n int safe_create_leading_directories(char *path);\n int safe_create_leading_directories_const(const char *path);\n char *enter_repo(char *path, int strict);\ndiff --git a/path.c b/path.c\nindex 42898e0..8a0a674 100644\n--- a/path.c\n+++ b/path.c\n@@ -311,16 +311,23 @@ char *enter_repo(char *path, int strict)\n \treturn NULL;\n }\n \n-int adjust_shared_perm(const char *path)\n+int set_shared_perm(const char *path, int mode)\n {\n \tstruct stat st;\n-\tint mode, tweak, shared;\n+\tint tweak, shared, orig_mode;\n \n-\tif (!shared_repository)\n+\tif (!shared_repository) {\n+\t\tif (mode)\n+\t\t\treturn chmod(path, mode & ~S_IFMT);\n \t\treturn 0;\n-\tif (lstat(path, &st) < 0)\n-\t\treturn -1;\n-\tmode = st.st_mode;\n+\t}\n+\tif (!mode) {\n+\t\tif (lstat(path, &st) < 0)\n+\t\t\treturn -1;\n+\t\tmode = st.st_mode;\n+\t\torig_mode = mode;\n+\t} else\n+\t\torig_mode = 0;\n \tif (shared_repository < 0)\n \t\tshared = -shared_repository;\n \telse\n@@ -344,9 +351,9 @@ int adjust_shared_perm(const char *path)\n \t}\n \n \tif (((shared_repository < 0\n-\t ? (st.st_mode & (FORCE_DIR_SET_GID | 0777))\n-\t : (st.st_mode & mode)) != mode) &&\n-\t chmod(path, mode) < 0)\n+\t ? (orig_mode & (FORCE_DIR_SET_GID | 0777))\n+\t : (orig_mode & mode)) != mode) &&\n+\t chmod(path, (mode & ~S_IFMT)) < 0)\n \t\treturn -2;\n \treturn 0;\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8869488..5bfc36c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2263,7 +2263,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t *\n \t * The same holds for FAT formatted media.\n \t *\n-\t * When this succeeds, we just return 0. We have nothing\n+\t * When this succeeds, we just return; we have nothing\n \t * left to unlink.\n \t */\n \tif (ret && ret != EEXIST) {\n@@ -2280,7 +2280,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t}\n \n out:\n-\tif (chmod(filename, 0444) || adjust_shared_perm(filename))\n+\tif (set_shared_perm(filename, (S_IFREG|0444)))\n \t\treturn error(\"unable to set permission to '%s'\", filename);\n \treturn 0;\n }\n"},{"id":"109699","messageId":"200903281148.41982.johan@herland.net","threadId":"18516","inReplyTo":"7vskky18ds.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Move chmod(foo, 0444) into move_temp_to_file()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-28T10:48:41Z","receivedAt":"2009-03-28T10:48:41Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 28 March 2009, Junio C Hamano wrote:\n> Johan Herland writes:\n> > When writing out a loose object or a pack (index), move_temp_to_file()\n> > is called to finalize the resulting file. These files (loose files and\n> > packs) should all have permission mode 0444 (modulo\n> > adjust_shared_perm()). Therefore, instead of doing chmod(foo, 0444)\n> > explicitly from each callsite (or even forgetting to chmod() at all),\n> > do the chmod() call from within move_temp_to_file().\n>\n> I think you would need this on top.\n\nAgreed.\n\nThanks,\n\n..Johan\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109701","messageId":"200903281201.35409.johan@herland.net","threadId":"18516","inReplyTo":"7viqlu1837.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Resolve double chmod() in move_temp_to_file()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-03-28T11:01:35Z","receivedAt":"2009-03-28T11:01:35Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Saturday 28 March 2009, Junio C Hamano wrote:\n> Johan Herland writes:\n> > -\tif (chmod(filename, 0444) || adjust_shared_perm(filename))\n> > +\tif (chmod(filename, get_shared_perm(0444)))\n>\n> Your get_shared_perm() will end up feeding 0444 to S_ISDIR(), which would\n> most likely say \"no\" and cause real harm, but there is no guarantee that\n> we won't start checking S_ISREG() or other things in get_shared_perm()\n> later. I do not like this.\n\nYou are right.\n\n> How about doing it this way instead?\n>\n> One thing to note is that we seem to have been passing what we read from\n> st.st_mode, together with S_IFMT bits, to chmod(2); I do not think I've\n> seen any breakage reports on exotic systems (glibc on Linux seems to\n> ignore the higher bits), but from my reading of POSIX, I would not be\n> surprised if somebody's chmod(2) returned EINVAL.\n\nAgreed.\n\n> -- >8 --\n> set_shared_perm(): sometimes we know what the final mode bits should look\n> like\n>\n> adjust_shared_perm() first obtains the mode bits from lstat(2), expecting\n> to find what the result of applying user's umask is, and then tweaked it\n\ns/tweaked/tweaks/\n\n> as necessary. When the file to be adjusted is created with mkstemp(3),\n> however, the mode thusly obtained does not have anything to do with\n> usre's umask, and we would need to start from 0444 in such a case and\n\ns/usre/user/\n\n> there is no point running lstat(2) for such a path.\n>\n> This introduces a new API set_shared_perm() to bypass the lstat(2) and\n> instead force setting the mode bits to the desired value directly.\n> adjust_shared_perm() becomes a thin wrapper to the function.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n\n[...]\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 8869488..5bfc36c 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2263,7 +2263,7 @@ int move_temp_to_file(const char *tmpfile, const\n> char *filename) *\n> \t * The same holds for FAT formatted media.\n> \t *\n> -\t * When this succeeds, we just return 0. We have nothing\n> +\t * When this succeeds, we just return; we have nothing\n\nSmall nit: This belongs in the previous patch, doesn't it?\n\n\nAll in all, this looks very good. Please drop my second patch, and use this \ninstead.\n\n\nHave fun! :)\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"109785","messageId":"7vfxgwqdej.fsf@gitster.siamese.dyndns.org","threadId":"18516","inReplyTo":"200903281201.35409.johan@herland.net","subject":"Re: [PATCH 2/2] Resolve double chmod() in move_temp_to_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-29T20:31:32Z","receivedAt":"2009-03-29T20:31:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Saturday 28 March 2009, Junio C Hamano wrote:\n>> Johan Herland writes:\n>> > -\tif (chmod(filename, 0444) || adjust_shared_perm(filename))\n>> > +\tif (chmod(filename, get_shared_perm(0444)))\n>>\n>> Your get_shared_perm() will end up feeding 0444 to S_ISDIR(), which would\n>> most likely say \"no\" and cause real harm, but there is no guarantee that\n>> we won't start checking S_ISREG() or other things in get_shared_perm()\n>> later. I do not like this.\n\n... should have been s/and cause/and cause no/; I think you read it correctly, though...\n\n> ...\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 8869488..5bfc36c 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -2263,7 +2263,7 @@ int move_temp_to_file(const char *tmpfile, const\n>> char *filename) *\n>> \t * The same holds for FAT formatted media.\n>> \t *\n>> -\t * When this succeeds, we just return 0. We have nothing\n>> +\t * When this succeeds, we just return; we have nothing\n>\n> Small nit: This belongs in the previous patch, doesn't it?\n\nThanks for all the above fixes.\n"}]}