[BUG?] How to make a shared/restricted repo?

[BUG?] How to make a shared/restricted repo?

[Johan Herland]

Hi,

Some colleagues of mine are working on a "secret" project, and they want to 
create a central/server/integration repo that should be group-writable, but 
not at all accessible to anybody outside the group (i.e. files should be 
0660 ("-rw-rw----"), dirs should be 0770 ("drwxrws---")).

I started setting this up for them in the following manner:

  mkdir foo.git
  cd foo.git
  git init --bare --shared=group
  cd ..
  chgrp -R groupname foo.git
  chmod -R o-rwx foo.git

...and everything looks good, initially...

However, when I start pushing into this repo, the newly created files are 
readable to everybody (files are 0664 ("-rw-rw-r--"), dirs are 0775 
("drwxrwsr-x")).

Instead of "git init --bare --shared=group", I've tried using
  git init --bare --shared=0660
and even
  git init --bare &&
  git config core.sharedRepository 0660
but the result is still the same.

After reading the "--shared" section in the "git init" man page, this 
behaviour is unexpected, and after reading the "core.sharedRepository" 
section in the "git config" man page, the current behaviour is IMHO outright 
_wrong_. Quoting the "git config" man page:

  core.sharedRepository
    [...] When 0xxx, where 0xxx is an octal number, files in the repository
    will have this mode value. 0xxx will override user’s umask value, and
    thus, users with a safe umask (0077) can use this option. [...]

AFAICS, even when I set "core.sharedRepository" to 0660, files are still 
created 0664, which is not what the documentation indicates.

Are there other ways to create such shared-but-restricted repositories?


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [BUG?] How to make a shared/restricted repo?

[Brandon Casey]

Johan Herland wrote:
> Hi,
> 
> Some colleagues of mine are working on a "secret" project, and they want to 
> create a central/server/integration repo that should be group-writable, but 
> not at all accessible to anybody outside the group (i.e. files should be 
> 0660 ("-rw-rw----"), dirs should be 0770 ("drwxrws---")).
> 
> I started setting this up for them in the following manner:
> 
>   mkdir foo.git
>   cd foo.git
>   git init --bare --shared=group
>   cd ..
>   chgrp -R groupname foo.git
>   chmod -R o-rwx foo.git
> 
> ...and everything looks good, initially...
> 
> However, when I start pushing into this repo, the newly created files are 
> readable to everybody (files are 0664 ("-rw-rw-r--"), dirs are 0775 
> ("drwxrwsr-x")).

But nobody has access to anything under foo.git since you did
'chmod o-rwx foo.git' above.

Unless I'm missing something, I think you already have what you want.

-brandon

Re: [BUG?] How to make a shared/restricted repo?

[Johan Herland]

On Wednesday 25 March 2009, Brandon Casey wrote:
> Johan Herland wrote:
> > Some colleagues of mine are working on a "secret" project, and they
> > want to create a central/server/integration repo that should be
> > group-writable, but not at all accessible to anybody outside the group
> > (i.e. files should be 0660 ("-rw-rw----"), dirs should be 2770
> > ("drwxrws---")).
> >
> > I started setting this up for them in the following manner:
> >
> >   mkdir foo.git
> >   cd foo.git
> >   git init --bare --shared=group
> >   cd ..
> >   chgrp -R groupname foo.git
> >   chmod -R o-rwx foo.git
> >
> > ...and everything looks good, initially...
> >
> > However, when I start pushing into this repo, the newly created files
> > are readable to everybody (files are 0664 ("-rw-rw-r--"), dirs are 2775
> > ("drwxrwsr-x")).
>
> But nobody has access to anything under foo.git since you did
> 'chmod o-rwx foo.git' above.

Yes, it's hard (impossible???) for outside users to get at the files, since 
they reside in directories that are not readable to them. However, this does 
not at all hide the fact that:

1. The "chmod -R o-rwx" is a command I added myself. Nowhere in Git's 
documentation is it said that it is a good idea to run this command.

2. Preferably, when creating a 0660 repo, "git init" should automatically 
perform this chmod for you, in the same manner that it already sets the 
"set-gid" bit for group-shared repos.

> Unless I'm missing something, I think you already have what you want.

Maybe, but it certainly doesn't fill me with warm, fuzzy, secure feelings.

Am I being overly paranoid?


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [BUG?] How to make a shared/restricted repo?

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Johan Herland wrote:
>> Hi,
>> 
>> Some colleagues of mine are working on a "secret" project, and they want to 
>> create a central/server/integration repo that should be group-writable, but 
>> not at all accessible to anybody outside the group (i.e. files should be 
>> 0660 ("-rw-rw----"), dirs should be 0770 ("drwxrws---")).
>> 
>> I started setting this up for them in the following manner:
>> 
>>   mkdir foo.git
>>   cd foo.git
>>   git init --bare --shared=group
>>   cd ..
>>   chgrp -R groupname foo.git
>>   chmod -R o-rwx foo.git
>> 
>> ...and everything looks good, initially...
>> 
>> However, when I start pushing into this repo, the newly created files are 
>> readable to everybody (files are 0664 ("-rw-rw-r--"), dirs are 0775 
>> ("drwxrwsr-x")).
>
> But nobody has access to anything under foo.git since you did
> 'chmod o-rwx foo.git' above.
>
> Unless I'm missing something, I think you already have what you want.

The toplevel is never recreated so it should be Ok in practice.

The core.sharedrepository only loosens the effect of overtight umask
setting that a project member has.  But you can notice inconsistency when
you run "ls -l", which may bother you ;-)

Re: [BUG?] How to make a shared/restricted repo?

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>   core.sharedRepository
>     [...] When 0xxx, where 0xxx is an octal number, files in the repository
>     will have this mode value. 0xxx will override user’s umask value, and
>     thus, users with a safe umask (0077) can use this option. [...]

Traditionally, core.shreadrepository was used only to widen overtight
umask some paranoid users have that are antisocial in a group project
setting.  An object that happens to have created by a lenient member may
be readable by others, but another object created by a member with
stricter umask won't be, which means "core.sharedRepository = group" means
just that; it guarantees that it is at least usable by everybody in the
group even if there is somebody antisocial in the group, and it does not
say anything about accessibility by others at all.

You might like to try a patch like this (untested).

 path.c |   17 +++++------------
 1 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/path.c b/path.c
index e332b50..fbc7011 100644
--- a/path.c
+++ b/path.c
@@ -314,7 +314,7 @@ char *enter_repo(char *path, int strict)
 int adjust_shared_perm(const char *path)
 {
 	struct stat st;
-	int mode;
+	int mode, tweak;
 
 	if (!shared_repository)
 		return 0;
@@ -322,17 +322,10 @@ int adjust_shared_perm(const char *path)
 		return -1;
 	mode = st.st_mode;
 
-	if (shared_repository) {
-		int tweak = shared_repository;
-		if (!(mode & S_IWUSR))
-			tweak &= ~0222;
-		mode |= tweak;
-	} else {
-		/* Preserve old PERM_UMASK behaviour */
-		if (mode & S_IWUSR)
-			mode |= S_IWGRP;
-	}
-
+	tweak = shared_repository;
+	if (!(mode & S_IWUSR))
+		tweak &= ~0222;
+	mode = (mode & ~0777) | tweak;
 	if (S_ISDIR(mode)) {
 		mode |= FORCE_DIR_SET_GID;
 

Re: [BUG?] How to make a shared/restricted repo?

[Johan Herland]

On Wednesday 25 March 2009, Junio C Hamano wrote:
> You might like to try a patch like this (untested).
>
>  path.c |   17 +++++------------
>  1 files changed, 5 insertions(+), 12 deletions(-)

Thanks!

This works much better :)

However, there are still some questions/issues:

- t1301-shared-repo.sh fails:
    Oops, .git/HEAD is not 0664 but -rw-rw---- [...]
    * FAIL 3: shared=1 does not clear bits preset by umask 022
  (I guess this is expected, as your patch changes the assumptions)

- Loose objects and pack files are still world-readable.

- Shared repos are no longer world-readable by default (requires
  "--shared=all" to be world-readable). This might be
  confusing to old users with lenient umasks, and should probably be
  mentioned in the documentation. AFAICS, it also has nasty effects
  on existing repos with core.sharedRepository == 1. We should probably
  re-roll the patch adding a new keyword (e.g. "group-only"), instead of
  changing the semantics of existing keywords/modes.

- Files/dirs copied from template directory are still world-readable.
  (This is not a big deal at all)


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [BUG?] How to make a shared/restricted repo?

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Wednesday 25 March 2009, Junio C Hamano wrote:
>> You might like to try a patch like this (untested).
>>
>>  path.c |   17 +++++------------
>>  1 files changed, 5 insertions(+), 12 deletions(-)
>
> Thanks!
>
> This works much better :)
>
> However, there are still some questions/issues:
>
> - t1301-shared-repo.sh fails:
>     Oops, .git/HEAD is not 0664 but -rw-rw---- [...]
>     * FAIL 3: shared=1 does not clear bits preset by umask 022
>   (I guess this is expected, as your patch changes the assumptions)

I'd rather say the patch breaks people's expectations.

[PATCH/RFC 0/7] Restricting repository access (Was: [BUG?] How to make a shared/restricted repo?)

[Johan Herland]

On Wednesday 25 March 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > On Wednesday 25 March 2009, Junio C Hamano wrote:
> >> You might like to try a patch like this (untested).
> >>
> >>  path.c |   17 +++++------------
> >>  1 files changed, 5 insertions(+), 12 deletions(-)
> >
> > Thanks!
> >
> > This works much better :)
> >
> > However, there are still some questions/issues:
> >
> > - t1301-shared-repo.sh fails:
> >     Oops, .git/HEAD is not 0664 but -rw-rw---- [...]
> >     * FAIL 3: shared=1 does not clear bits preset by umask 022
> >   (I guess this is expected, as your patch changes the assumptions)
>
> I'd rather say the patch breaks people's expectations.

I thought some more about the current semantics, and came up with this
patch series, which replaces your original suggestion.

In short, I leave the core.sharedRepository semantics as is (i.e. it is
only used to _loosen_ repository permissions), and introduce a new
variable - core.restrictedRepository - that takes care of _tightening_
repository permissions. Its value is a permission mask that is applied
to the file mode in adjust_shared_perm()

The patch series is based on recent 'next', and the testsuite passes
after each individual patch.

Here is a short rundown of the individual patches:

1. Clarify existing documentation to reflect the current semantics of
   core.sharedRepository and "git init --shared". Even if the rest of
   the series is rejected, I hope this can make it in some form.

2. Minor cleanup in path.c:adjust_shared_perm(). This is pretty much
   your original patch with any functional changes removed.

3. Introduce core.restrictedRepository. Adds git_config_perm_mask()
   for parsing the config value, and changes adjust_shared_perm() to
   apply the permission mask. Includes documentation of the new config
   variable.

4. Add "--restricted" to "git init". Heavily modeled on the existing
   "--shared" option. Includes documentation of the new option.

5. Add tests for the functionality introduced in #3 and #4.

6. Apply adjusted repository permissions in "git init" when copying
   templates into the new repo.

7. Apply restricted permissions to loose objects and pack files. This
   ensures that loose objects and pack files do not get permissions
   that are more liberal than the rest of the repository.


Have fun!

...Johan


Johan Herland (7):
  Clarify documentation on permissions in shared repositories
  Cleanup: Remove unnecessary if-else clause
  Introduce core.restrictedRepository for restricting repository
    permissions
  git-init: Introduce --restricted for restricting repository access
  Add tests for "core.restrictedRepository" and "git init --restricted"
  git-init: Apply correct mode bits to template files in
    shared/restricted repo
  Apply restricted permissions to loose objects and pack files

 Documentation/config.txt   |   41 ++++++++++++-
 Documentation/git-init.txt |   50 +++++++++++++++--
 builtin-init-db.c          |   31 +++++++++-
 cache.h                    |    8 +++
 environment.c              |    1 +
 fast-import.c              |    4 +-
 http-push.c                |    2 +-
 http-walker.c              |    2 +-
 index-pack.c               |    4 +-
 path.c                     |   22 +++----
 setup.c                    |   36 ++++++++++++
 sha1_file.c                |    2 +-
 t/t0001-init.sh            |   24 +++++++-
 t/t1304-restricted-repo.sh |  132 ++++++++++++++++++++++++++++++++++++++++++++
 14 files changed, 323 insertions(+), 36 deletions(-)
 create mode 100755 t/t1304-restricted-repo.sh

[PATCH/RFC 1/7] Clarify documentation on permissions in shared repositories

[Johan Herland]

The "core.sharedRepository" config variable (and, by extension, the
"--shared" argument to "git init") can be used to loosen repository
permissions for users with a safe umask, but it can not be used to
tighten repository permissions for users with a more lenient umask.

This patch updates the documentation to clarify the current behaviour.

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/config.txt   |   15 +++++++++++----
 Documentation/git-init.txt |   12 ++++++++----
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 089569a..d5befd5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -294,10 +294,17 @@ core.sharedRepository::
 	repository will be readable by all users, additionally to being
 	group-shareable. When 'umask' (or 'false'), git will use permissions
 	reported by umask(2). When '0xxx', where '0xxx' is an octal number,
-	files in the repository will have this mode value. '0xxx' will override
-	user's umask value, and thus, users with a safe umask (0077) can use
-	this option. Examples: '0660' is equivalent to 'group'. '0640' is a
-	repository that is group-readable but not group-writable.
+	files in the repository will have (at least) this mode value. '0xxx'
+	will override a safer umask value, and thus, users with a safe umask
+	(0077) can use this option to loosen the repository permissions.
+	Examples: '0660' is equivalent to 'group'. '0640' is a repository
+	that is group-readable but not group-writable (unless umask allows
+	group-writability).
+	Note: Even when not set to 'umask' (or 'false') this option is still
+	combined with the umask to produce the actual mode value. For
+	example, if umask is 0022, setting 'group' (or '0660') will not make
+	the repository non world-readable (the actual mode value will in fact
+	be '0664').
 	See linkgit:git-init[1]. False by default.
 
 core.warnAmbiguousRefs::
diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 71749c0..bddc01b 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -45,6 +45,7 @@ repository.  When specified, the config variable "core.sharedRepository" is
 set so that files and directories under `$GIT_DIR` are created with the
 requested permissions.  When not specified, git will use permissions reported
 by umask(2).
+When specified, the permissions will still be no stricter than the umask.
 
 The option can have the following values, defaulting to 'group' if no value
 is given:
@@ -58,11 +59,14 @@ is given:
  - 'all' (or 'world' or 'everybody'): Same as 'group', but make the repository
    readable by all users.
 
- - '0xxx': '0xxx' is an octal number and each file will have mode '0xxx'
+ - '0xxx': '0xxx' is an octal number and each file will have (at least) this
+   mode value.
    Any option except 'umask' can be set using this option. '0xxx' will
-   override users umask(2) value, and thus, users with a safe umask (0077)
-   can use this option. '0640' will create a repository which is group-readable
-   but not writable. '0660' is equivalent to 'group'.
+   override a safer umask(2) value (but not a more permissive umask), and
+   thus, users with a safe umask (e.g. 0077) can use this option to loosen
+   repository permissions. '0640' will create a repository which is
+   group-readable but not writable (unless umask allows group-writability).
+   '0660' is equivalent to 'group'.
 
 By default, the configuration flag receive.denyNonFastForwards is enabled
 in shared repositories, so that you cannot force a non fast-forwarding push
-- 
1.6.2.1.473.g92672

[PATCH/RFC 2/7] Cleanup: Remove unnecessary if-else clause

[Johan Herland]

Signed-off-by: Johan Herland <johan@herland.net>
---
 path.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/path.c b/path.c
index e332b50..28e50d3 100644
--- a/path.c
+++ b/path.c
@@ -322,16 +322,10 @@ int adjust_shared_perm(const char *path)
 		return -1;
 	mode = st.st_mode;
 
-	if (shared_repository) {
-		int tweak = shared_repository;
-		if (!(mode & S_IWUSR))
-			tweak &= ~0222;
-		mode |= tweak;
-	} else {
-		/* Preserve old PERM_UMASK behaviour */
-		if (mode & S_IWUSR)
-			mode |= S_IWGRP;
-	}
+	int tweak = shared_repository;
+	if (!(mode & S_IWUSR))
+		tweak &= ~0222;
+	mode |= tweak;
 
 	if (S_ISDIR(mode)) {
 		mode |= FORCE_DIR_SET_GID;
-- 
1.6.2.1.473.g92672

[PATCH/RFC 3/7] Introduce core.restrictedRepository for restricting repository permissions

[Johan Herland]

core.sharedRepository is only used to loosen/expand permissions (beyond the
user's umask). In order to tighten/restrict permissions (stricter than the
umask), we add a new configuration variable that serves as an additional
mask on the permissions of created files and directories.

The new config variable - core.restrictedRepository - can be set to an octal
number, interpreted as an additional mask to be applied to the permissions
of created files and directories. It can also be set to "umask" (or "false",
equivalent to "0000", i.e. no more restrictive than the user's umask), "user"
(or "true", equivalent to "0077", i.e. dropping read- and write-permissions
for group and others), or "group" (equivalent to "0007", i.e. dropping read-
and write-permissions for others). If set with no value, it defaults to
"user" (the most secure/restricted state).

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/config.txt |   26 ++++++++++++++++++++++++++
 cache.h                  |    7 +++++++
 environment.c            |    1 +
 path.c                   |   14 ++++++++------
 setup.c                  |   36 ++++++++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5befd5..0f2dd5c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -305,8 +305,34 @@ core.sharedRepository::
 	example, if umask is 0022, setting 'group' (or '0660') will not make
 	the repository non world-readable (the actual mode value will in fact
 	be '0664').
+	See "core.restrictedRepository" for how to restrict repository access
+	below/within the umask.
 	See linkgit:git-init[1]. False by default.
 
+core.restrictedRepository::
+	When 'user' (or 'true'), the repository is made inaccessible to
+	anybody but the user owning the repository. When 'group', repository
+	access is restricted to the members of the repository's group.
+	When 'umask' (or 'false'), git will use permissions reported by
+	umask(2). When '0xxx', where '0xxx' is an octal number, files in the
+	repository will the specified mode bits masked off. '0xxx' will
+	override a more lenient umask value, and thus, users with a loose
+	umask (e.g. 0022) can use this option to further restrict the
+	repository permissions.
+	Examples: '0007' is equivalent to 'group'. '0077' is a repository
+	that is only accessible to the repository owner.
+	Note: Even when not set to 'umask' (or 'false') this option is still
+	combined with the umask to produce the actual mask value. For
+	example, if umask is 0077, setting 'group' (or '0007') will not make
+	the repository group-accessible (the calculated mode mask will in
+	fact be '0077', restricting access to the repository owner only).
+	See "core.sharedRepository" for how to loosen repository access
+	beyond the umask.
+	Example: To set up a group-shared repository that is inaccessible to
+	all non-members, set both "core.sharedRepository" and
+	"core.restrictedRepository" to "group".
+	False by default.
+
 core.warnAmbiguousRefs::
 	If true, git will warn you if the ref name you passed it is ambiguous
 	and might match multiple refs in the .git/refs/ tree. True by default.
diff --git a/cache.h b/cache.h
index 9cf5a13..4730f33 100644
--- a/cache.h
+++ b/cache.h
@@ -508,6 +508,7 @@ extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
+extern int restricted_repository;
 extern const char *apply_default_whitespace;
 extern int zlib_compression_level;
 extern int core_compression_level;
@@ -622,7 +623,13 @@ enum sharedrepo {
 	PERM_GROUP          = 0660,
 	PERM_EVERYBODY      = 0664,
 };
+enum restrictedrepo {
+	PERM_MASK_UMASK     = 0000,
+	PERM_MASK_GROUP     = 0007,
+	PERM_MASK_USER      = 0077,
+};
 int git_config_perm(const char *var, const char *value);
+int git_config_perm_mask(const char *var, const char *value);
 int adjust_shared_perm(const char *path);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
diff --git a/environment.c b/environment.c
index 4696885..9591710 100644
--- a/environment.c
+++ b/environment.c
@@ -25,6 +25,7 @@ int repository_format_version;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
 int shared_repository = PERM_UMASK;
+int restricted_repository = PERM_MASK_UMASK;
 const char *apply_default_whitespace;
 int zlib_compression_level = Z_BEST_SPEED;
 int core_compression_level;
diff --git a/path.c b/path.c
index 28e50d3..093c87c 100644
--- a/path.c
+++ b/path.c
@@ -316,25 +316,27 @@ int adjust_shared_perm(const char *path)
 	struct stat st;
 	int mode;
 
-	if (!shared_repository)
+	if (!shared_repository && !restricted_repository)
 		return 0;
 	if (lstat(path, &st) < 0)
 		return -1;
 	mode = st.st_mode;
 
-	int tweak = shared_repository;
 	if (!(mode & S_IWUSR))
-		tweak &= ~0222;
-	mode |= tweak;
+		restricted_repository |= 0222;
+
+	/* add shared_repository and subtract restricted_repository */
+	mode |= shared_repository;
+	mode &= ~restricted_repository;
 
 	if (S_ISDIR(mode)) {
 		mode |= FORCE_DIR_SET_GID;
 
 		/* Copy read bits to execute bits */
-		mode |= (shared_repository & 0444) >> 2;
+		mode |= (mode & 0444) >> 2;
 	}
 
-	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
+	if (st.st_mode != mode && chmod(path, mode) < 0)
 		return -2;
 	return 0;
 }
diff --git a/setup.c b/setup.c
index 6c2deda..97685d4 100644
--- a/setup.c
+++ b/setup.c
@@ -459,12 +459,48 @@ int git_config_perm(const char *var, const char *value)
 	return i & 0666;
 }
 
+int git_config_perm_mask(const char *var, const char *value)
+{
+	int i;
+	char *endptr;
+
+	if (value == NULL)
+		return PERM_MASK_USER;
+
+	if (!strcmp(value, "umask"))
+		return PERM_MASK_UMASK;
+	if (!strcmp(value, "group"))
+		return PERM_MASK_GROUP;
+	if (!strcmp(value, "user"))
+		return PERM_MASK_USER;
+
+	/* Parse octal numbers */
+	i = strtol(value, &endptr, 8);
+
+	/* If not an octal number, maybe true/false? */
+	if (*endptr != 0)
+		return git_config_bool(var, value) ? PERM_MASK_USER :
+			PERM_MASK_UMASK;
+
+	/* A filemode mask value was given: 0xxx */
+
+	if (i & 0700)
+		die("Problem with core.restrictedRepository filemode mask "
+		    "value (0%.3o).\nThe owner of files must always have "
+		    "read and write permissions.", i);
+
+	/* Mask filemode mask value. Should only affect r/w/x flags for g/o */
+	return i & 0077;
+}
+
 int check_repository_format_version(const char *var, const char *value, void *cb)
 {
 	if (strcmp(var, "core.repositoryformatversion") == 0)
 		repository_format_version = git_config_int(var, value);
 	else if (strcmp(var, "core.sharedrepository") == 0)
 		shared_repository = git_config_perm(var, value);
+	else if (strcmp(var, "core.restrictedrepository") == 0)
+		restricted_repository = git_config_perm_mask(var, value);
 	else if (strcmp(var, "core.bare") == 0) {
 		is_bare_repository_cfg = git_config_bool(var, value);
 		if (is_bare_repository_cfg == 1)
-- 
1.6.2.1.473.g92672

[PATCH/RFC 4/7] git-init: Introduce --restricted for restricting repository access

[Johan Herland]

"--restricted" does for "core.restrictedRepository" what "--shared" does for
"core.sharedRepository".

Signed-off-by: Johan Herland <johan@herland.net>
---
 Documentation/config.txt   |    2 +-
 Documentation/git-init.txt |   38 +++++++++++++++++++++++++++++++++++++-
 builtin-init-db.c          |   22 ++++++++++++++++++----
 3 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0f2dd5c..08f8068 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -331,7 +331,7 @@ core.restrictedRepository::
 	Example: To set up a group-shared repository that is inaccessible to
 	all non-members, set both "core.sharedRepository" and
 	"core.restrictedRepository" to "group".
-	False by default.
+	See linkgit:git-init[1]. False by default.
 
 core.warnAmbiguousRefs::
 	If true, git will warn you if the ref name you passed it is ambiguous
diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index bddc01b..2a431c2 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -8,7 +8,7 @@ git-init - Create an empty git repository or reinitialize an existing one
 
 SYNOPSIS
 --------
-'git init' [-q | --quiet] [--bare] [--template=<template_directory>] [--shared[=<permissions>]]
+'git init' [-q | --quiet] [--bare] [--template=<template_directory>] [--shared[=<permissions>]] [--restricted[=<permissions>]]
 
 
 OPTIONS
@@ -72,6 +72,42 @@ By default, the configuration flag receive.denyNonFastForwards is enabled
 in shared repositories, so that you cannot force a non fast-forwarding push
 into it.
 
+--restricted[={false|true|umask|group|user|0xxx}]::
+
+Specify that the git repository is to be restricted according to the given
+permission mask.  This allows you to more finely control access to the
+repository.  When specified, the config variable "core.restrictedRepository"
+is set so that files and directories under `$GIT_DIR` are created with the
+restrictions in the given mask.  When not specified, git will use permissions
+reported by umask(2). When specified, the permissions will still be no more
+lenient than the umask allows.
+
+The option can have the following values, defaulting to 'user' if no value
+is given:
+
+ - 'umask' (or 'false'): Use permissions reported by umask(2). The default,
+   when `--restricted` is not specified.
+
+ - 'group': Make the repository accessible only to members of the group
+   owning the repository.
+
+ - 'user' (or 'true'): Make the repository inaccessible to anybody but the
+   repository owner.
+
+ - '0xxx': '0xxx' is an octal number and each file will have (at least) these
+   mode bits masked off the repository permission. '0xxx' will override a
+   more lenient umask(2) value (but not a stricter/safer umask), and thus,
+   users with a lenient umask (e.g. 0022) can use this option to tighten
+   repository permissions. '0000' is equivalent to 'umask', '0007' is
+   equivalent to 'group', and '0077' is equivalent to 'user'.
+   '0027' will create a repository which is group-readable (unless overridden
+   by the current umask), but not group-writable, and inaccessible to others.
+
+You can combine `--shared` and `--restricted` to finely control the access to
+the repository. For example, specifying `--shared=group --restricted=group`
+will ensure that the repository is group-readable and group-writable, and
+also non world-readable and non world-writable.
+
 --
 
 
diff --git a/builtin-init-db.c b/builtin-init-db.c
index fc63d0f..8e7fa2d 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -19,6 +19,7 @@
 
 static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
+static int init_restricted_repository = -1;
 
 static void safe_create_dir(const char *dir, int share)
 {
@@ -29,7 +30,7 @@ static void safe_create_dir(const char *dir, int share)
 		}
 	}
 	else if (share && adjust_shared_perm(dir))
-		die("Could not make %s writable by group", dir);
+		die("Could not set proper permissions on %s", dir);
 }
 
 static void copy_templates_1(char *path, int baselen,
@@ -196,12 +197,14 @@ static int create_default_files(const char *template_path)
 	is_bare_repository_cfg = init_is_bare_repository;
 	if (init_shared_repository != -1)
 		shared_repository = init_shared_repository;
+	if (init_restricted_repository != -1)
+		restricted_repository = init_restricted_repository;
 
 	/*
 	 * We would have created the above under user's umask -- under
 	 * shared-repository settings, we would need to fix them up.
 	 */
-	if (shared_repository) {
+	if (shared_repository || restricted_repository) {
 		adjust_shared_perm(get_git_dir());
 		adjust_shared_perm(git_path("refs"));
 		adjust_shared_perm(git_path("refs/heads"));
@@ -321,11 +324,17 @@ int init_db(const char *template_dir, unsigned int flags)
 		git_config_set("core.sharedrepository", buf);
 		git_config_set("receive.denyNonFastforwards", "true");
 	}
+	if (restricted_repository) {
+		char buf[5];
+		sprintf(buf, "%04o", restricted_repository);
+		git_config_set("core.restrictedrepository", buf);
+	}
 
 	if (!(flags & INIT_DB_QUIET))
-		printf("%s%s Git repository in %s/\n",
+		printf("%s%s%s Git repository in %s/\n",
 		       reinit ? "Reinitialized existing" : "Initialized empty",
 		       shared_repository ? " shared" : "",
+		       restricted_repository ? " restricted" : "",
 		       get_git_dir());
 
 	return 0;
@@ -363,7 +372,7 @@ static int guess_repository_type(const char *git_dir)
 }
 
 static const char init_db_usage[] =
-"git init [-q | --quiet] [--bare] [--template=<template-directory>] [--shared[=<permissions>]]";
+"git init [-q | --quiet] [--bare] [--template=<template-directory>] [--shared[=<permissions>]] [--restricted[=<permissions>]]";
 
 /*
  * If you want to, you can share the DB area with any number of branches.
@@ -391,6 +400,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			init_shared_repository = PERM_GROUP;
 		else if (!prefixcmp(arg, "--shared="))
 			init_shared_repository = git_config_perm("arg", arg+9);
+		else if (!strcmp(arg, "--restricted"))
+			init_restricted_repository = PERM_MASK_USER;
+		else if (!prefixcmp(arg, "--restricted="))
+			init_restricted_repository =
+				git_config_perm_mask("arg", arg+13);
 		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
 			flags |= INIT_DB_QUIET;
 		else
-- 
1.6.2.1.473.g92672

[PATCH/RFC 5/7] Add tests for "core.restrictedRepository" and "git init --restricted"

[Johan Herland]

These tests are based on - and analogous to - the existing tests for
"core.sharedRepository" and "git init --shared"

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t0001-init.sh            |   24 +++++++-
 t/t1304-restricted-repo.sh |  132 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 153 insertions(+), 3 deletions(-)
 create mode 100755 t/t1304-restricted-repo.sh

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 5ac0a27..639a88d 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -167,7 +167,7 @@ test_expect_success 'init with --template (blank)' '
 	! test -f template-blank/.git/info/exclude
 '
 
-test_expect_success 'init --bare/--shared overrides system/global config' '
+test_expect_success 'init --bare/--shared/--restricted overrides system/global config' '
 	(
 		HOME="`pwd`" &&
 		export HOME &&
@@ -175,13 +175,16 @@ test_expect_success 'init --bare/--shared overrides system/global config' '
 		unset GIT_CONFIG_NOGLOBAL &&
 		git config -f "$test_config" core.bare false &&
 		git config -f "$test_config" core.sharedRepository 0640 &&
+		git config -f "$test_config" core.restrictedRepository 0027 &&
 		mkdir init-bare-shared-override &&
 		cd init-bare-shared-override &&
-		git init --bare --shared=0666
+		git init --bare --shared=0644 --restricted=0022
 	) &&
 	check_config init-bare-shared-override true unset &&
-	test x0666 = \
+	test x0644 = \
 	x`git config -f init-bare-shared-override/config core.sharedRepository`
+	test x0022 = \
+	x`git config -f init-bare-shared-override/config core.restrictedRepository`
 '
 
 test_expect_success 'init honors global core.sharedRepository' '
@@ -199,4 +202,19 @@ test_expect_success 'init honors global core.sharedRepository' '
 	x`git config -f shared-honor-global/.git/config core.sharedRepository`
 '
 
+test_expect_success 'init honors global core.restrictedRepository' '
+	(
+		HOME="`pwd`" &&
+		export HOME &&
+		test_config="$HOME"/.gitconfig &&
+		unset GIT_CONFIG_NOGLOBAL &&
+		git config -f "$test_config" core.restrictedRepository 0077 &&
+		mkdir restricted-honor-global &&
+		cd restricted-honor-global &&
+		git init
+	) &&
+	test x0077 = \
+	x`git config -f restricted-honor-global/.git/config core.restrictedRepository`
+'
+
 test_done
diff --git a/t/t1304-restricted-repo.sh b/t/t1304-restricted-repo.sh
new file mode 100755
index 0000000..012cdf1
--- /dev/null
+++ b/t/t1304-restricted-repo.sh
@@ -0,0 +1,132 @@
+#!/bin/sh
+#
+# Copied and modified from t1301-shared-repo.sh
+#
+
+test_description='Test restricted repository initialization'
+
+. ./test-lib.sh
+
+# Remove a default ACL from the test dir if possible.
+setfacl -k . 2>/dev/null
+
+# User must have r/w permissions to the repo -> failure on --restricted=0600
+test_expect_success 'restricted = 0600 (faulty permission u-rw)' '
+	mkdir sub && (
+		cd sub && git init --restricted=0600
+	)
+	ret="$?"
+	rm -rf sub
+	test $ret != "0"
+'
+
+modebits () {
+	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
+}
+
+for u in 0007 0077
+do
+	test_expect_success POSIXPERM "restricted=group does not clear bits preset by umask $u" '
+		mkdir sub && (
+			cd sub &&
+			umask $u &&
+			git init --restricted=group &&
+			test "0007" = "$(git config core.restrictedrepository)"
+		) &&
+		actual=$(ls -l sub/.git/HEAD)
+		case "$u$actual" in
+		0007-rw-rw----*)
+			: happy
+			;;
+		0077-rw-------*)
+			: happy
+			;;
+		*)
+			echo Oops, .git/HEAD is not 06x0 but $actual
+			false
+			;;
+		esac
+	'
+	rm -rf sub
+done
+
+test_expect_success 'restricted=user' '
+	mkdir sub &&
+	cd sub &&
+	git init --restricted=user &&
+	test "0077" = "$(git config core.restrictedrepository)"
+'
+
+test_expect_success POSIXPERM 'update-server-info honors core.restrictedRepository' '
+	: > a1 &&
+	git add a1 &&
+	test_tick &&
+	git commit -m a1 &&
+	umask 0277 &&
+	git update-server-info &&
+	actual="$(ls -l .git/info/refs)" &&
+	case "$actual" in
+	-r--------*)
+		: happy
+		;;
+	*)
+		echo Oops, .git/info/refs is not 0400
+		false
+		;;
+	esac
+'
+
+for u in	0000:rw-rw-rw- \
+		0002:rw-rw-r-- \
+		0007:rw-rw---- \
+		0027:rw-r----- \
+		0077:rw-------
+do
+	x=$(expr "$u" : ".*:\([rw-]*\)") &&
+	y=$(echo "$x" | sed -e "s/w/-/g") &&
+	u=$(expr "$u" : "\([0-7]*\)"); test $? -le 1 &&
+	git config core.restrictedrepository "$u" &&
+	umask 0222 &&
+	test_expect_success POSIXPERM "shared = $u ($y) ro" '
+
+		rm -f .git/info/refs &&
+		git update-server-info &&
+		actual="$(modebits .git/info/refs)" &&
+		test "x$actual" = "x-$y" || {
+			ls -lt .git/info
+			false
+		}
+	'
+
+	umask 0000 &&
+	test_expect_success POSIXPERM "shared = $u ($x) rw" '
+
+		rm -f .git/info/refs &&
+		git update-server-info &&
+		actual="$(modebits .git/info/refs)" &&
+		test "x$actual" = "x-$x" || {
+			ls -lt .git/info
+			false
+		}
+
+	'
+
+done
+
+test_expect_success POSIXPERM 'git reflog expire honors core.restrictedRepository' '
+	umask 0000
+	git config core.restrictedRepository group &&
+	git reflog expire --all &&
+	actual="$(ls -l .git/logs/refs/heads/master)" &&
+	case "$actual" in
+	-rw-rw----*)
+		: happy
+		;;
+	*)
+		echo Ooops, .git/logs/refs/heads/master is not 0660 [$actual]
+		false
+		;;
+	esac
+'
+
+test_done
-- 
1.6.2.1.473.g92672

[PATCH/RFC 6/7] git-init: Apply correct mode bits to template files in shared/restricted repo

[Johan Herland]

Template dirs/files are copied into the new repo before the config file is
parsed. This causes them to be created with default permissions (i.e. from
umask). When "--shared" and/or "--restricted" is given, this patch teaches
git-init to initialize the global variables controlling permissions on
created files (shared_repository and/or restricted_repository) _before_
the templates are copied. They are thus created with the appropriate
permissions.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin-init-db.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 8e7fa2d..fb88f65 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -187,6 +187,15 @@ static int create_default_files(const char *template_path)
 	safe_create_dir(git_path("refs/heads"), 1);
 	safe_create_dir(git_path("refs/tags"), 1);
 
+	/*
+	 * Set up shared/restricted_repository before copying templates, so
+	 * that they are created with appropriate permissions.
+	 */
+	if (init_shared_repository != -1)
+		shared_repository = init_shared_repository;
+	if (init_restricted_repository != -1)
+		restricted_repository = init_restricted_repository;
+
 	/* First copy the templates -- we might have the default
 	 * config file there, in which case we would want to read
 	 * from it after installing.
-- 
1.6.2.1.473.g92672

[PATCH/RFC 7/7] Apply restricted permissions to loose objects and pack files

[Johan Herland]

Loose objects and pack files are normally created with mode 0444, but in a
repository that uses core.restrictedRepository to restrict file modes, we
further limit permissions on loose objects and pack files, according to the
restrictedRepository setting.

Signed-off-by: Johan Herland <johan@herland.net>
---
 cache.h       |    1 +
 fast-import.c |    4 ++--
 http-push.c   |    2 +-
 http-walker.c |    2 +-
 index-pack.c  |    4 ++--
 sha1_file.c   |    2 +-
 6 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 4730f33..82e562b 100644
--- a/cache.h
+++ b/cache.h
@@ -509,6 +509,7 @@ extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
 extern int restricted_repository;
+#define PERM_SHA1_FILE (0444 & ~restricted_repository)
 extern const char *apply_default_whitespace;
 extern int zlib_compression_level;
 extern int core_compression_level;
diff --git a/fast-import.c b/fast-import.c
index beeac0d..feafe6f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -902,8 +902,8 @@ static char *keep_pack(char *curr_index_name)
 	static const char *keep_msg = "fast-import";
 	int keep_fd;
 
-	chmod(pack_data->pack_name, 0444);
-	chmod(curr_index_name, 0444);
+	chmod(pack_data->pack_name, PERM_SHA1_FILE);
+	chmod(curr_index_name, PERM_SHA1_FILE);
 
 	keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);
 	if (keep_fd < 0)
diff --git a/http-push.c b/http-push.c
index 6ce5a1d..e33044f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -748,7 +748,7 @@ static void finish_request(struct transfer_request *request)
 			aborted = 1;
 		}
 	} else if (request->state == RUN_FETCH_LOOSE) {
-		fchmod(request->local_fileno, 0444);
+		fchmod(request->local_fileno, PERM_SHA1_FILE);
 		close(request->local_fileno); request->local_fileno = -1;
 
 		if (request->curl_result != CURLE_OK &&
diff --git a/http-walker.c b/http-walker.c
index 0dbad3c..a0dd5d2 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -231,7 +231,7 @@ static void finish_object_request(struct object_request *obj_req)
 {
 	struct stat st;
 
-	fchmod(obj_req->local, 0444);
+	fchmod(obj_req->local, PERM_SHA1_FILE);
 	close(obj_req->local); obj_req->local = -1;
 
 	if (obj_req->http_code == 416) {
diff --git a/index-pack.c b/index-pack.c
index 7546822..c82e60a 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -825,7 +825,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 			die("cannot store pack file");
 	}
 	if (from_stdin)
-		chmod(final_pack_name, 0444);
+		chmod(final_pack_name, PERM_SHA1_FILE);
 
 	if (final_index_name != curr_index_name) {
 		if (!final_index_name) {
@@ -836,7 +836,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		if (move_temp_to_file(curr_index_name, final_index_name))
 			die("cannot store index file");
 	}
-	chmod(final_index_name, 0444);
+	chmod(final_index_name, PERM_SHA1_FILE);
 
 	if (!from_stdin) {
 		printf("%s\n", sha1_to_hex(sha1));
diff --git a/sha1_file.c b/sha1_file.c
index a354f06..ad63fe1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2272,7 +2272,7 @@ static void close_sha1_file(int fd)
 {
 	if (fsync_object_files)
 		fsync_or_die(fd, "sha1 file");
-	fchmod(fd, 0444);
+	fchmod(fd, PERM_SHA1_FILE);
 	if (close(fd) != 0)
 		die("unable to write sha1 file");
 }
-- 
1.6.2.1.473.g92672

Re: [BUG?] How to make a shared/restricted repo?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johan Herland <johan@herland.net> writes:
>
>> On Wednesday 25 March 2009, Junio C Hamano wrote:
>>> You might like to try a patch like this (untested).
>>>
>>>  path.c |   17 +++++------------
>>>  1 files changed, 5 insertions(+), 12 deletions(-)
>>
>> Thanks!
>>
>> This works much better :)
>>
>> However, there are still some questions/issues:
>>
>> - t1301-shared-repo.sh fails:
>>     Oops, .git/HEAD is not 0664 but -rw-rw---- [...]
>>     * FAIL 3: shared=1 does not clear bits preset by umask 022
>>   (I guess this is expected, as your patch changes the assumptions)
>
> I'd rather say the patch breaks people's expectations.

How about doing it like this, instead?

This fixes the behaviour of octal notation to how it is defined in the
documentation, while keeping the traditional "loosen only" semantics
intact for "group" and "everybody".

Three main points of this patch are:

 - For an explicit octal notation, the internal shared_repository variable
   is set to a negative value, so that we can tell "group" (which is to
   "OR" in 0660) and 0660 (which is to "SET" to 0660);

 - git-init did not set shared_repository variable early enough to affect
   the initial creation of many files, notably copied templates and the
   configuration.  We set it very early when a command-line option
   specifies a custom value.

 - Loose object creation codepath first letsmkstemp() to create a new
   temporary file; depending on systems, this gets 0660 or 0600, which
   does not have anything to do with user's umask.  To compensate for
   this, close_sha1_file() unconditionally doing fchmod(0444).  For the
   traditional "loosen-only", adjust_shared_perm() call after it would be
   a no-op (the mode is already loose enough), but with the new behaviour
   it makes a difference.

I think there are more places you could sprinkle adjust_shared_perm() for
packfiles and other things.  I didn't check, not because I wasn't
uninterested, but because I am more interested in getting what
adjust_shared_perm() itself does right.

 builtin-init-db.c |   12 ++++++++++--
 path.c            |   33 ++++++++++++++++++---------------
 setup.c           |    5 ++---
 sha1_file.c       |    3 +++
 4 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index fc63d0f..4e02b33 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -194,6 +194,8 @@ static int create_default_files(const char *template_path)
 
 	git_config(git_default_config, NULL);
 	is_bare_repository_cfg = init_is_bare_repository;
+
+	/* reading existing config may have overwrote it */
 	if (init_shared_repository != -1)
 		shared_repository = init_shared_repository;
 
@@ -312,12 +314,15 @@ int init_db(const char *template_dir, unsigned int flags)
 		 * and compatibility values for PERM_GROUP and
 		 * PERM_EVERYBODY.
 		 */
-		if (shared_repository == PERM_GROUP)
+		if (shared_repository < 0)
+			/* force to the mode value */
+			sprintf(buf, "0%o", -shared_repository);
+		else if (shared_repository == PERM_GROUP)
 			sprintf(buf, "%d", OLD_PERM_GROUP);
 		else if (shared_repository == PERM_EVERYBODY)
 			sprintf(buf, "%d", OLD_PERM_EVERYBODY);
 		else
-			sprintf(buf, "0%o", shared_repository);
+			die("oops");
 		git_config_set("core.sharedrepository", buf);
 		git_config_set("receive.denyNonFastforwards", "true");
 	}
@@ -397,6 +402,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			usage(init_db_usage);
 	}
 
+	if (init_shared_repository != -1)
+		shared_repository = init_shared_repository;
+
 	/*
 	 * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR
 	 * without --bare.  Catch the error early.
diff --git a/path.c b/path.c
index e332b50..9026b6e 100644
--- a/path.c
+++ b/path.c
@@ -314,33 +314,36 @@ char *enter_repo(char *path, int strict)
 int adjust_shared_perm(const char *path)
 {
 	struct stat st;
-	int mode;
+	int mode, tweak, shared;
 
 	if (!shared_repository)
 		return 0;
 	if (lstat(path, &st) < 0)
 		return -1;
 	mode = st.st_mode;
-
-	if (shared_repository) {
-		int tweak = shared_repository;
-		if (!(mode & S_IWUSR))
-			tweak &= ~0222;
+	if (shared_repository < 0)
+		shared = -shared_repository;
+	else
+		shared = shared_repository;
+	tweak = shared;
+
+	if (!(mode & S_IWUSR))
+		tweak &= ~0222;
+	if (shared_repository < 0)
+		mode = (mode & ~0777) | tweak;
+	else
 		mode |= tweak;
-	} else {
-		/* Preserve old PERM_UMASK behaviour */
-		if (mode & S_IWUSR)
-			mode |= S_IWGRP;
-	}
 
 	if (S_ISDIR(mode)) {
-		mode |= FORCE_DIR_SET_GID;
-
 		/* Copy read bits to execute bits */
-		mode |= (shared_repository & 0444) >> 2;
+		mode |= FORCE_DIR_SET_GID;
+		mode |= (shared & 0444) >> 2;
 	}
 
-	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
+	if (((shared_repository < 0
+	      ? (st.st_mode & (FORCE_DIR_SET_GID | 0777))
+	      : (st.st_mode & mode)) != mode) &&
+	    chmod(path, mode) < 0)
 		return -2;
 	return 0;
 }
diff --git a/setup.c b/setup.c
index 6c2deda..a430238 100644
--- a/setup.c
+++ b/setup.c
@@ -434,7 +434,7 @@ int git_config_perm(const char *var, const char *value)
 
 	/*
 	 * Treat values 0, 1 and 2 as compatibility cases, otherwise it is
-	 * a chmod value.
+	 * a chmod value to restrict to.
 	 */
 	switch (i) {
 	case PERM_UMASK:               /* 0 */
@@ -456,7 +455,7 @@ int git_config_perm(const char *var, const char *value)
 	 * Mask filemode value. Others can not get write permission.
 	 * x flags for directories are handled separately.
 	 */
-	return i & 0666;
+	return -(i & 0666);
 }
 
 int check_repository_format_version(const char *var, const char *value, void *cb)
diff --git a/sha1_file.c b/sha1_file.c
index 54972f9..b4c12c4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2393,6 +2393,9 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 				tmpfile, strerror(errno));
 	}
 
+	if (adjust_shared_perm(tmpfile))
+		die("unable to set permission to '%s'", tmpfile);
+
 	return move_temp_to_file(tmpfile, filename);
 }
 

Re: [BUG?] How to make a shared/restricted repo?

[Johan Herland]

On Thursday 26 March 2009, Junio C Hamano wrote:
> How about doing it like this, instead?

Looks good, and is obviously much less intrusive than my attempt.

There's still one issue as compared to my series: Hook scripts in
.git/hooks lose their executable bit when copied from template dir.
You probably need to do some kind of special x-bit handling, similar
to what's already done for directories.

Other than that:
Tested-by: Johan Herland <johan@herland.net>

> I think there are more places you could sprinkle adjust_shared_perm() for
> packfiles and other things.  I didn't check, not because I wasn't
> uninterested, but because I am more interested in getting what
> adjust_shared_perm() itself does right.

Here is my #7 patch, re-rolled on top of your patch to call
adjust_shared_perm():

---
 fast-import.c |    2 ++
 http-push.c   |    1 +
 http-walker.c |    1 +
 index-pack.c  |    2 ++
 4 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index beeac0d..d73ee71 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -903,7 +903,9 @@ static char *keep_pack(char *curr_index_name)
        int keep_fd;
 
        chmod(pack_data->pack_name, 0444);
+       adjust_shared_perm(pack_data->pack_name);
        chmod(curr_index_name, 0444);
+       adjust_shared_perm(curr_index_name);
 
        keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);
        if (keep_fd < 0)
diff --git a/http-push.c b/http-push.c
index 6ce5a1d..a5acabf 100644
--- a/http-push.c
+++ b/http-push.c
@@ -750,6 +750,7 @@ static void finish_request(struct transfer_request *request)
        } else if (request->state == RUN_FETCH_LOOSE) {
                fchmod(request->local_fileno, 0444);
                close(request->local_fileno); request->local_fileno = -1;
+               adjust_shared_perm(request->tmpfile);
 
                if (request->curl_result != CURLE_OK &&
                    request->http_code != 416) {
diff --git a/http-walker.c b/http-walker.c
index 0dbad3c..24cfc45 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -233,6 +233,7 @@ static void finish_object_request(struct object_request *obj_req)
 
        fchmod(obj_req->local, 0444);
        close(obj_req->local); obj_req->local = -1;
+       adjust_shared_perm(obj_req->tmpfile);
 
        if (obj_req->http_code == 416) {
                fprintf(stderr, "Warning: requested range invalid; we may already have all the data.\n");
diff --git a/index-pack.c b/index-pack.c
index 7546822..7abe3f0 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -826,6 +826,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
        }
        if (from_stdin)
                chmod(final_pack_name, 0444);
+               adjust_shared_perm(final_pack_name);
 
        if (final_index_name != curr_index_name) {
                if (!final_index_name) {
@@ -837,6 +838,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
                        die("cannot store index file");
        }
        chmod(final_index_name, 0444);
+       adjust_shared_perm(final_index_name);
 
        if (!from_stdin) {
                printf("%s\n", sha1_to_hex(sha1));
-- 
1.6.2.1.473.g92672

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [BUG?] How to make a shared/restricted repo?

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Thursday 26 March 2009, Junio C Hamano wrote:
>> How about doing it like this, instead?
>
> Looks good, and is obviously much less intrusive than my attempt.
>
> There's still one issue as compared to my series: Hook scripts in
> .git/hooks lose their executable bit when copied from template dir.
> You probably need to do some kind of special x-bit handling, similar
> to what's already done for directories.
>
> Other than that:
> Tested-by: Johan Herland <johan@herland.net>

Sorry to invalidate your Tested-by, but here is a re-roll using a slightly
different strategy.

To fix the loose object codepath, the earlier patch added a call to
adjust_shared_perm() to write_loose_object() function, but after looking
at your 7th patch, I realized that the pattern of file creation inside
$GIT_DIR typically is to first create a temporary file, write to it, and
then finish it off by calling move_temp_to_file().  The true purpose of
the function is to "finalize the file being created", and it is misnamed
in that it describes how its implementation does it currently (i.e. by
renaming the temporary file to its final name), but it makes perfect sense
to call adjust_shared_perm() inside it as a part of finalization.  I think
this should cover the codepaths your 7th patch fixed without actually
touching them.

Could you eyeball and re-test it?

-- >8 --
[PATCH] "core.sharedrepository = 0mode" should set, not loosen

This fixes the behaviour of octal notation to how it is defined in the
documentation, while keeping the traditional "loosen only" semantics
intact for "group" and "everybody".

Three main points of this patch are:

 - For an explicit octal notation, the internal shared_repository variable
   is set to a negative value, so that we can tell "group" (which is to
   "OR" in 0660) and 0660 (which is to "SET" to 0660);

 - git-init did not set shared_repository variable early enough to affect
   the initial creation of many files, notably copied templates and the
   configuration.  We set it very early when a command-line option
   specifies a custom value.

 - Many codepaths create files inside $GIT_DIR by various codepaths that
   involve mkstemp(), and then call move_temp_to_file() to rename it to
   its final destination.  We can add adjust_shared_perm() call here; for
   the traditional "loosen-only", this would be a no-op for many codepaths
   because the mode is already loose enough, but with the new behaviour it
   makes a difference.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-init-db.c      |   12 ++++++++++--
 path.c                 |   36 +++++++++++++++++++++---------------
 setup.c                |    4 ++--
 sha1_file.c            |    8 +++++++-
 t/t1301-shared-repo.sh |   37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 77 insertions(+), 20 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index ee3911f..8199e5d 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -195,6 +195,8 @@ static int create_default_files(const char *template_path)
 
 	git_config(git_default_config, NULL);
 	is_bare_repository_cfg = init_is_bare_repository;
+
+	/* reading existing config may have overwrote it */
 	if (init_shared_repository != -1)
 		shared_repository = init_shared_repository;
 
@@ -313,12 +315,15 @@ int init_db(const char *template_dir, unsigned int flags)
 		 * and compatibility values for PERM_GROUP and
 		 * PERM_EVERYBODY.
 		 */
-		if (shared_repository == PERM_GROUP)
+		if (shared_repository < 0)
+			/* force to the mode value */
+			sprintf(buf, "0%o", -shared_repository);
+		else if (shared_repository == PERM_GROUP)
 			sprintf(buf, "%d", OLD_PERM_GROUP);
 		else if (shared_repository == PERM_EVERYBODY)
 			sprintf(buf, "%d", OLD_PERM_EVERYBODY);
 		else
-			sprintf(buf, "0%o", shared_repository);
+			die("oops");
 		git_config_set("core.sharedrepository", buf);
 		git_config_set("receive.denyNonFastforwards", "true");
 	}
@@ -398,6 +403,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			usage(init_db_usage);
 	}
 
+	if (init_shared_repository != -1)
+		shared_repository = init_shared_repository;
+
 	/*
 	 * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR
 	 * without --bare.  Catch the error early.
diff --git a/path.c b/path.c
index e332b50..42898e0 100644
--- a/path.c
+++ b/path.c
@@ -314,33 +314,39 @@ char *enter_repo(char *path, int strict)
 int adjust_shared_perm(const char *path)
 {
 	struct stat st;
-	int mode;
+	int mode, tweak, shared;
 
 	if (!shared_repository)
 		return 0;
 	if (lstat(path, &st) < 0)
 		return -1;
 	mode = st.st_mode;
-
-	if (shared_repository) {
-		int tweak = shared_repository;
-		if (!(mode & S_IWUSR))
-			tweak &= ~0222;
+	if (shared_repository < 0)
+		shared = -shared_repository;
+	else
+		shared = shared_repository;
+	tweak = shared;
+
+	if (!(mode & S_IWUSR))
+		tweak &= ~0222;
+	if (mode & S_IXUSR)
+		/* Copy read bits to execute bits */
+		tweak |= (tweak & 0444) >> 2;
+	if (shared_repository < 0)
+		mode = (mode & ~0777) | tweak;
+	else
 		mode |= tweak;
-	} else {
-		/* Preserve old PERM_UMASK behaviour */
-		if (mode & S_IWUSR)
-			mode |= S_IWGRP;
-	}
 
 	if (S_ISDIR(mode)) {
-		mode |= FORCE_DIR_SET_GID;
-
 		/* Copy read bits to execute bits */
-		mode |= (shared_repository & 0444) >> 2;
+		mode |= (shared & 0444) >> 2;
+		mode |= FORCE_DIR_SET_GID;
 	}
 
-	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
+	if (((shared_repository < 0
+	      ? (st.st_mode & (FORCE_DIR_SET_GID | 0777))
+	      : (st.st_mode & mode)) != mode) &&
+	    chmod(path, mode) < 0)
 		return -2;
 	return 0;
 }
diff --git a/setup.c b/setup.c
index 6c2deda..ebd60de 100644
--- a/setup.c
+++ b/setup.c
@@ -434,7 +434,7 @@ int git_config_perm(const char *var, const char *value)
 
 	/*
 	 * Treat values 0, 1 and 2 as compatibility cases, otherwise it is
-	 * a chmod value.
+	 * a chmod value to restrict to.
 	 */
 	switch (i) {
 	case PERM_UMASK:               /* 0 */
@@ -456,7 +456,7 @@ int git_config_perm(const char *var, const char *value)
 	 * Mask filemode value. Others can not get write permission.
 	 * x flags for directories are handled separately.
 	 */
-	return i & 0666;
+	return -(i & 0666);
 }
 
 int check_repository_format_version(const char *var, const char *value, void *cb)
diff --git a/sha1_file.c b/sha1_file.c
index a07aa4e..45987bd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2243,11 +2243,15 @@ static void write_sha1_file_prepare(const void *buf, unsigned long len,
 }
 
 /*
- * Move the just written object into its final resting place
+ * Move the just written object into its final resting place.
+ * NEEDSWORK: this should be renamed to finalize_temp_file() as
+ * "moving" is only a part of what it does, when no patch between
+ * master to pu changes the call sites of this function.
  */
 int move_temp_to_file(const char *tmpfile, const char *filename)
 {
 	int ret = 0;
+
 	if (link(tmpfile, filename))
 		ret = errno;
 
@@ -2275,6 +2279,8 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 		/* FIXME!!! Collision check here ? */
 	}
 
+	if (adjust_shared_perm(filename))
+		return error("unable to set permission to '%s'", filename);
 	return 0;
 }
 
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 653362b..d459854 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -126,4 +126,41 @@ test_expect_success 'git reflog expire honors core.sharedRepository' '
 	esac
 '
 
+test_expect_success 'forced modes' '
+	mkdir -p templates/hooks &&
+	echo update-server-info >templates/hooks/post-update &&
+	chmod +x templates/hooks/post-update &&
+	echo : >random-file &&
+	mkdir new &&
+	(
+		cd new &&
+		umask 002 &&
+		git init --shared=0660 --template=../templates &&
+		>frotz &&
+		git add frotz &&
+		git commit -a -m initial &&
+		git repack
+	) &&
+	find new/.git -print |
+	xargs ls -ld >actual &&
+
+	# Everything must be unaccessible to others
+	test -z "$(sed -n -e "/^.......---/d" actual)" &&
+
+	# All directories must have 2770
+	test -z "$(sed -n -e "/^drwxrws---/d" -e "/^d/p" actual)" &&
+
+	# post-update hook must be 0770
+	test -z "$(sed -n -e "/post-update/{
+		/^-rwxrwx---/d
+		p
+	}" actual)" &&
+
+	# All files inside objects must be 0440
+	test -z "$(sed -n -e "/objects\//{
+		/^d/d
+		/^-r--r-----/d
+	}" actual)"
+'
+
 test_done
-- 
1.6.2.1.394.g50949

Re: [BUG?] How to make a shared/restricted repo?

[Johan Herland]

On Thursday 26 March 2009, Junio C Hamano wrote:
> To fix the loose object codepath, the earlier patch added a call to
> adjust_shared_perm() to write_loose_object() function, but after looking
> at your 7th patch, I realized that the pattern of file creation inside
> $GIT_DIR typically is to first create a temporary file, write to it, and
> then finish it off by calling move_temp_to_file().  The true purpose of
> the function is to "finalize the file being created", and it is misnamed
> in that it describes how its implementation does it currently (i.e. by
> renaming the temporary file to its final name), but it makes perfect
> sense to call adjust_shared_perm() inside it as a part of finalization. 
> I think this should cover the codepaths your 7th patch fixed without
> actually touching them.

Yes, with one exception:

For the two cases index-pack.c, the chmod(foo, 0444) happens AFTER the
corresponding call to move_temp_to_file(xyzzy, foo). The chmod() in
adjust_shared_perms() would thus be overridden by the chmod(foo, 0444),
which is not what we want. In both cases, I think the chmod(foo, 0444)
can safely be moved up above the call to move_temp_to_file(). Something
like this (although I'm not sure about the semantics of 'from_stdin'):

diff --git a/index-pack.c b/index-pack.c
index 7546822..d289b6a 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -815,6 +815,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		}
 	}
 
+	if (from_stdin)
+		chmod(final_pack_name, 0444);
 	if (final_pack_name != curr_pack_name) {
 		if (!final_pack_name) {
 			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
@@ -824,9 +826,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		if (move_temp_to_file(curr_pack_name, final_pack_name))
 			die("cannot store pack file");
 	}
-	if (from_stdin)
-		chmod(final_pack_name, 0444);
 
+	chmod(final_index_name, 0444);
 	if (final_index_name != curr_index_name) {
 		if (!final_index_name) {
 			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
@@ -836,7 +837,6 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		if (move_temp_to_file(curr_index_name, final_index_name))
 			die("cannot store index file");
 	}
-	chmod(final_index_name, 0444);
 
 	if (!from_stdin) {
 		printf("%s\n", sha1_to_hex(sha1));


> Could you eyeball and re-test it?

> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Tested-by: Johan Herland <johan@herland.net>

> --- a/builtin-init-db.c
> +++ b/builtin-init-db.c
> @@ -195,6 +195,8 @@ static int create_default_files(const char
> *template_path)
>
>  	git_config(git_default_config, NULL);
>  	is_bare_repository_cfg = init_is_bare_repository;
> +
> +	/* reading existing config may have overwrote it */

s/overwrote/overwritten/

Otherwise OK, AFAICS.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [BUG?] How to make a shared/restricted repo?

[Johannes Sixt]

Johan Herland schrieb:
> For the two cases index-pack.c, the chmod(foo, 0444) happens AFTER the
> corresponding call to move_temp_to_file(xyzzy, foo). The chmod() in
> adjust_shared_perms() would thus be overridden by the chmod(foo, 0444),
> which is not what we want. In both cases, I think the chmod(foo, 0444)
> can safely be moved up above the call to move_temp_to_file(). Something
> like this (although I'm not sure about the semantics of 'from_stdin'):
> 
> diff --git a/index-pack.c b/index-pack.c
> index 7546822..d289b6a 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -815,6 +815,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		}
>  	}
>  
> +	if (from_stdin)
> +		chmod(final_pack_name, 0444);
>  	if (final_pack_name != curr_pack_name) {
>  		if (!final_pack_name) {
>  			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
> @@ -824,9 +826,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		if (move_temp_to_file(curr_pack_name, final_pack_name))
>  			die("cannot store pack file");
>  	}
> -	if (from_stdin)
> -		chmod(final_pack_name, 0444);
>  
> +	chmod(final_index_name, 0444);
>  	if (final_index_name != curr_index_name) {
>  		if (!final_index_name) {
>  			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
> @@ -836,7 +837,6 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		if (move_temp_to_file(curr_index_name, final_index_name))
>  			die("cannot store index file");
>  	}
> -	chmod(final_index_name, 0444);
>  
>  	if (!from_stdin) {
>  		printf("%s\n", sha1_to_hex(sha1));

You certainly meant to use the curr_*_name variants in the chmod lines,
no? This effectively undoes 33b65030, but that is not so good: On Windows
we cannot rename read-only files.

-- Hannes

Re: [BUG?] How to make a shared/restricted repo?

[Johan Herland]

On Thursday 26 March 2009, Johannes Sixt wrote:
> You certainly meant to use the curr_*_name variants in the chmod lines,
> no?

Oops. Yes. My bad.

> This effectively undoes 33b65030, but that is not so good: On Windows
> we cannot rename read-only files.

What about this: Add a "mode" argument to move_temp_to_file(), used to
pass in the 0444, and then within move_temp_to_file() we can do the
chmod(foo, 0444) _after_ renaming the file, but _before_ calling
adjust_shared_perm().

Something like this on top of Junio's patch:

----------------------->8-------------8<-------------------
diff --git a/cache.h b/cache.h
index 9cf5a13..9b28b98 100644
--- a/cache.h
+++ b/cache.h
@@ -652,7 +652,8 @@ extern int do_check_packed_object_crc;
 
 extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type);
 
-extern int move_temp_to_file(const char *tmpfile, const char *filename);
+/* Set mode to -1 if you don't want to explicitly change the file mode */
+extern int finalize_temp_file(const char *tmpfile, const char *filename, int mode);
 
 extern int has_sha1_pack(const unsigned char *sha1);
 extern int has_sha1_file(const unsigned char *sha1);
diff --git a/fast-import.c b/fast-import.c
index beeac0d..a5556e9 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -902,9 +902,6 @@ static char *keep_pack(char *curr_index_name)
 	static const char *keep_msg = "fast-import";
 	int keep_fd;
 
-	chmod(pack_data->pack_name, 0444);
-	chmod(curr_index_name, 0444);
-
 	keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);
 	if (keep_fd < 0)
 		die("cannot create keep file");
@@ -914,12 +911,12 @@ static char *keep_pack(char *curr_index_name)
 
 	snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
 		 get_object_directory(), sha1_to_hex(pack_data->sha1));
-	if (move_temp_to_file(pack_data->pack_name, name))
+	if (finalize_temp_file(pack_data->pack_name, name, 0444))
 		die("cannot store pack file");
 
 	snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
 		 get_object_directory(), sha1_to_hex(pack_data->sha1));
-	if (move_temp_to_file(curr_index_name, name))
+	if (finalize_temp_file(curr_index_name, name, 0444))
 		die("cannot store index file");
 	return name;
 }
diff --git a/http-push.c b/http-push.c
index 6ce5a1d..5be6600 100644
--- a/http-push.c
+++ b/http-push.c
@@ -748,7 +748,6 @@ static void finish_request(struct transfer_request *request)
 			aborted = 1;
 		}
 	} else if (request->state == RUN_FETCH_LOOSE) {
-		fchmod(request->local_fileno, 0444);
 		close(request->local_fileno); request->local_fileno = -1;
 
 		if (request->curl_result != CURLE_OK &&
@@ -769,9 +768,10 @@ static void finish_request(struct transfer_request *request)
 				unlink(request->tmpfile);
 			} else {
 				request->rename =
-					move_temp_to_file(
+					finalize_temp_file(
 						request->tmpfile,
-						request->filename);
+						request->filename,
+						0444);
 				if (request->rename == 0) {
 					request->obj->flags |= (LOCAL | REMOTE);
 				}
@@ -794,8 +794,9 @@ static void finish_request(struct transfer_request *request)
 
 			fclose(request->local_stream);
 			request->local_stream = NULL;
-			if (!move_temp_to_file(request->tmpfile,
-					       request->filename)) {
+			if (!finalize_temp_file(request->tmpfile,
+					request->filename,
+					-1)) {
 				target = (struct packed_git *)request->userData;
 				target->pack_size = pack_size;
 				lst = &repo->packs;
@@ -1007,7 +1008,7 @@ static int fetch_index(unsigned char *sha1)
 	free(url);
 	fclose(indexfile);
 
-	return move_temp_to_file(tmpfile, filename);
+	return finalize_temp_file(tmpfile, filename, -1);
 }
 
 static int setup_index(unsigned char *sha1)
diff --git a/http-walker.c b/http-walker.c
index 0dbad3c..7da78e1 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -231,7 +231,6 @@ static void finish_object_request(struct object_request *obj_req)
 {
 	struct stat st;
 
-	fchmod(obj_req->local, 0444);
 	close(obj_req->local); obj_req->local = -1;
 
 	if (obj_req->http_code == 416) {
@@ -254,7 +253,7 @@ static void finish_object_request(struct object_request *obj_req)
 		return;
 	}
 	obj_req->rename =
-		move_temp_to_file(obj_req->tmpfile, obj_req->filename);
+		finalize_temp_file(obj_req->tmpfile, obj_req->filename, 0444);
 
 	if (obj_req->rename == 0)
 		walker_say(obj_req->walker, "got %s\n", sha1_to_hex(obj_req->sha1));
@@ -429,7 +428,7 @@ static int fetch_index(struct walker *walker, struct alt_base *repo, unsigned ch
 
 	fclose(indexfile);
 
-	return move_temp_to_file(tmpfile, filename);
+	return finalize_temp_file(tmpfile, filename, -1);
 }
 
 static int setup_index(struct walker *walker, struct alt_base *repo, unsigned char *sha1)
@@ -788,7 +787,7 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 	target->pack_size = ftell(packfile);
 	fclose(packfile);
 
-	ret = move_temp_to_file(tmpfile, filename);
+	ret = finalize_temp_file(tmpfile, filename, -1);
 	if (ret)
 		return ret;
 
diff --git a/index-pack.c b/index-pack.c
index 7546822..237acd1 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -821,10 +821,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 				 get_object_directory(), sha1_to_hex(sha1));
 			final_pack_name = name;
 		}
-		if (move_temp_to_file(curr_pack_name, final_pack_name))
+		if (finalize_temp_file(curr_pack_name, final_pack_name, 0444))
 			die("cannot store pack file");
-	}
-	if (from_stdin)
+	} else if (from_stdin)
 		chmod(final_pack_name, 0444);
 
 	if (final_index_name != curr_index_name) {
@@ -833,10 +832,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 				 get_object_directory(), sha1_to_hex(sha1));
 			final_index_name = name;
 		}
-		if (move_temp_to_file(curr_index_name, final_index_name))
+		if (finalize_temp_file(curr_index_name, final_index_name, 0444))
 			die("cannot store index file");
-	}
-	chmod(final_index_name, 0444);
+	} else
+		chmod(final_index_name, 0444);
 
 	if (!from_stdin) {
 		printf("%s\n", sha1_to_hex(sha1));
diff --git a/sha1_file.c b/sha1_file.c
index 12e0dfd..3c611f4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2215,13 +2215,8 @@ static void write_sha1_file_prepare(const void *buf, unsigned long len,
 	git_SHA1_Final(sha1, &c);
 }
 
-/*
- * Move the just written object into its final resting place.
- * NEEDSWORK: this should be renamed to finalize_temp_file() as
- * "moving" is only a part of what it does, when no patch between
- * master to pu changes the call sites of this function.
- */
-int move_temp_to_file(const char *tmpfile, const char *filename)
+/* Move the just written object into its final resting place. */
+int finalize_temp_file(const char *tmpfile, const char *filename, int mode)
 {
 	int ret = 0;
 
@@ -2252,8 +2247,10 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 		/* FIXME!!! Collision check here ? */
 	}
 
+	if (mode > 0 && chmod(filename, mode))
+		return error("unable to set permission %04o on '%s'", mode, filename);
 	if (adjust_shared_perm(filename))
-		return error("unable to set permission to '%s'", filename);
+		return error("unable to adjust permission on '%s'", filename);
 	return 0;
 }
 
@@ -2278,7 +2275,6 @@ static void close_sha1_file(int fd)
 {
 	if (fsync_object_files)
 		fsync_or_die(fd, "sha1 file");
-	fchmod(fd, 0444);
 	if (close(fd) != 0)
 		die("error when closing sha1 file (%s)", strerror(errno));
 }
@@ -2386,7 +2382,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 				tmpfile, strerror(errno));
 	}
 
-	return move_temp_to_file(tmpfile, filename);
+	return finalize_temp_file(tmpfile, filename, 0444);
 }
 
 int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
----------------------->8-------------8<-------------------


In the above patch, I've passed mode == -1 to finalize_temp_file() from all
callsites where there was no corresponding (f)chmod(foo, 0444). However,
after looking at the context (these are all either packs or loose objects),
I'm wondering if we shouldn't pass mode == 0444 for all of these. At which
point we could replace the above patch with this much simpler version:

----------------------->8-------------8<-------------------
diff --git a/fast-import.c b/fast-import.c
index beeac0d..1df3e86 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -902,9 +902,6 @@ static char *keep_pack(char *curr_index_name)
 	static const char *keep_msg = "fast-import";
 	int keep_fd;
 
-	chmod(pack_data->pack_name, 0444);
-	chmod(curr_index_name, 0444);
-
 	keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);
 	if (keep_fd < 0)
 		die("cannot create keep file");
diff --git a/http-push.c b/http-push.c
index 6ce5a1d..e465b20 100644
--- a/http-push.c
+++ b/http-push.c
@@ -748,7 +748,6 @@ static void finish_request(struct transfer_request *request)
 			aborted = 1;
 		}
 	} else if (request->state == RUN_FETCH_LOOSE) {
-		fchmod(request->local_fileno, 0444);
 		close(request->local_fileno); request->local_fileno = -1;
 
 		if (request->curl_result != CURLE_OK &&
diff --git a/http-walker.c b/http-walker.c
index 0dbad3c..c5a3ea3 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -231,7 +231,6 @@ static void finish_object_request(struct object_request *obj_req)
 {
 	struct stat st;
 
-	fchmod(obj_req->local, 0444);
 	close(obj_req->local); obj_req->local = -1;
 
 	if (obj_req->http_code == 416) {
diff --git a/index-pack.c b/index-pack.c
index 7546822..6e93ee6 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -823,8 +823,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		}
 		if (move_temp_to_file(curr_pack_name, final_pack_name))
 			die("cannot store pack file");
-	}
-	if (from_stdin)
+	} else if (from_stdin)
 		chmod(final_pack_name, 0444);
 
 	if (final_index_name != curr_index_name) {
@@ -835,8 +834,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		}
 		if (move_temp_to_file(curr_index_name, final_index_name))
 			die("cannot store index file");
-	}
-	chmod(final_index_name, 0444);
+	} else
+		chmod(final_index_name, 0444);
 
 	if (!from_stdin) {
 		printf("%s\n", sha1_to_hex(sha1));
diff --git a/sha1_file.c b/sha1_file.c
index 12e0dfd..87ac53b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2252,7 +2252,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 		/* FIXME!!! Collision check here ? */
 	}
 
-	if (adjust_shared_perm(filename))
+	if (chmod(filename, 0444) || adjust_shared_perm(filename))
 		return error("unable to set permission to '%s'", filename);
 	return 0;
 }
@@ -2278,7 +2278,6 @@ static void close_sha1_file(int fd)
 {
 	if (fsync_object_files)
 		fsync_or_die(fd, "sha1 file");
-	fchmod(fd, 0444);
 	if (close(fd) != 0)
 		die("error when closing sha1 file (%s)", strerror(errno));
 }
----------------------->8-------------8<-------------------


(We could also add an optional "mode" argument to adjust_shared_perm(), to
get rid of the double chmod().)


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [BUG?] How to make a shared/restricted repo?

[Johannes Sixt]

Johan Herland schrieb:
> In the above patch, I've passed mode == -1 to finalize_temp_file() from all
> callsites where there was no corresponding (f)chmod(foo, 0444). However,
> after looking at the context (these are all either packs or loose objects),
> I'm wondering if we shouldn't pass mode == 0444 for all of these. At which
> point we could replace the above patch with this much simpler version:

Indeed!

> (We could also add an optional "mode" argument to adjust_shared_perm(), to
> get rid of the double chmod().)

And I think you should do that, otherwise you have a short time window
where the permissions of a pack or loose object is less restrictive than
you want.

-- Hannes

[PATCH 0/2] chmod cleanup (Was: [BUG?] How to make a shared/restricted repo?)

[Johan Herland]

On Thursday 26 March 2009, Johannes Sixt wrote:
> Johan Herland schrieb:
> > In the above patch, I've passed mode == -1 to finalize_temp_file()
> > from all callsites where there was no corresponding (f)chmod(foo,
> > 0444). However, after looking at the context (these are all either
> > packs or loose objects), I'm wondering if we shouldn't pass mode ==
> > 0444 for all of these. At which point we could replace the above
> > patch with this much simpler version:
>
> Indeed!
>
> > (We could also add an optional "mode" argument to
> > adjust_shared_perm(), to get rid of the double chmod().)
>
> And I think you should do that, otherwise you have a short time
> window where the permissions of a pack or loose object is less
> restrictive than you want.

Ok, here's a cleaned-up series on top of Junio's patch. It should resolve
the chmod()-after-adjust_shared_perm() issue in Junio's patch, as well as
the rename-after-chmod Windows issue reported by Hannes.

The first patch is the second alternative I sent in an earlier mail.
The second patch resolves the double chmod() left by the first patch.


Johan Herland (2):
  Move chmod(foo, 0444) into move_temp_to_file()
  Resolve double chmod() in move_temp_to_file()

 cache.h       |    1 +
 fast-import.c |    3 ---
 http-push.c   |    1 -
 http-walker.c |    1 -
 index-pack.c  |    7 +++----
 path.c        |   26 +++++++++++++++++++-------
 sha1_file.c   |    3 +--
 7 files changed, 24 insertions(+), 18 deletions(-)


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH 1/2] Move chmod(foo, 0444) into move_temp_to_file()

[Johan Herland]

When writing out a loose object or a pack (index), move_temp_to_file() is
called to finalize the resulting file. These files (loose files and packs)
should all have permission mode 0444 (modulo adjust_shared_perm()).
Therefore, instead of doing chmod(foo, 0444) explicitly from each callsite
(or even forgetting to chmod() at all), do the chmod() call from within
move_temp_to_file().

Signed-off-by: Johan Herland <johan@herland.net>
---
 fast-import.c |    3 ---
 http-push.c   |    1 -
 http-walker.c |    1 -
 index-pack.c  |    7 +++----
 sha1_file.c   |    3 +--
 5 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index db44da3..23c496d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -903,9 +903,6 @@ static char *keep_pack(char *curr_index_name)
 	static const char *keep_msg = "fast-import";
 	int keep_fd;
 
-	chmod(pack_data->pack_name, 0444);
-	chmod(curr_index_name, 0444);
-
 	keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);
 	if (keep_fd < 0)
 		die("cannot create keep file");
diff --git a/http-push.c b/http-push.c
index 6ce5a1d..e465b20 100644
--- a/http-push.c
+++ b/http-push.c
@@ -748,7 +748,6 @@ static void finish_request(struct transfer_request *request)
 			aborted = 1;
 		}
 	} else if (request->state == RUN_FETCH_LOOSE) {
-		fchmod(request->local_fileno, 0444);
 		close(request->local_fileno); request->local_fileno = -1;
 
 		if (request->curl_result != CURLE_OK &&
diff --git a/http-walker.c b/http-walker.c
index 0dbad3c..c5a3ea3 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -231,7 +231,6 @@ static void finish_object_request(struct object_request *obj_req)
 {
 	struct stat st;
 
-	fchmod(obj_req->local, 0444);
 	close(obj_req->local); obj_req->local = -1;
 
 	if (obj_req->http_code == 416) {
diff --git a/index-pack.c b/index-pack.c
index 7546822..6e93ee6 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -823,8 +823,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		}
 		if (move_temp_to_file(curr_pack_name, final_pack_name))
 			die("cannot store pack file");
-	}
-	if (from_stdin)
+	} else if (from_stdin)
 		chmod(final_pack_name, 0444);
 
 	if (final_index_name != curr_index_name) {
@@ -835,8 +834,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		}
 		if (move_temp_to_file(curr_index_name, final_index_name))
 			die("cannot store index file");
-	}
-	chmod(final_index_name, 0444);
+	} else
+		chmod(final_index_name, 0444);
 
 	if (!from_stdin) {
 		printf("%s\n", sha1_to_hex(sha1));
diff --git a/sha1_file.c b/sha1_file.c
index 12e0dfd..87ac53b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2252,7 +2252,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 		/* FIXME!!! Collision check here ? */
 	}
 
-	if (adjust_shared_perm(filename))
+	if (chmod(filename, 0444) || adjust_shared_perm(filename))
 		return error("unable to set permission to '%s'", filename);
 	return 0;
 }
@@ -2278,7 +2278,6 @@ static void close_sha1_file(int fd)
 {
 	if (fsync_object_files)
 		fsync_or_die(fd, "sha1 file");
-	fchmod(fd, 0444);
 	if (close(fd) != 0)
 		die("error when closing sha1 file (%s)", strerror(errno));
 }
-- 
1.6.1.2.461.g5bad6

Re: [PATCH 1/2] Move chmod(foo, 0444) into move_temp_to_file()

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> When writing out a loose object or a pack (index), move_temp_to_file() is
> called to finalize the resulting file. These files (loose files and packs)
> should all have permission mode 0444 (modulo adjust_shared_perm()).
> Therefore, instead of doing chmod(foo, 0444) explicitly from each callsite
> (or even forgetting to chmod() at all), do the chmod() call from within
> move_temp_to_file().
>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---

I think you would need this on top.

-- >8 --
move_temp_to_file(): do not forget to chmod() in "Coda hack" codepath

Now move_temp_to_file() is responsible for doing everything that is
necessary to turn a tempfile in $GIT_DIR into its final form, it must make
sure "Coda hack" codepath correctly makes the file read-only.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_file.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 3bd20e7..8869488 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2268,7 +2268,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 	 */
 	if (ret && ret != EEXIST) {
 		if (!rename(tmpfile, filename))
-			return 0;
+			goto out;
 		ret = errno;
 	}
 	unlink(tmpfile);
@@ -2279,6 +2279,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 		/* FIXME!!! Collision check here ? */
 	}
 
+out:
 	if (chmod(filename, 0444) || adjust_shared_perm(filename))
 		return error("unable to set permission to '%s'", filename);
 	return 0;

Re: [PATCH 1/2] Move chmod(foo, 0444) into move_temp_to_file()

[Johan Herland]

On Saturday 28 March 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > When writing out a loose object or a pack (index), move_temp_to_file()
> > is called to finalize the resulting file. These files (loose files and
> > packs) should all have permission mode 0444 (modulo
> > adjust_shared_perm()). Therefore, instead of doing chmod(foo, 0444)
> > explicitly from each callsite (or even forgetting to chmod() at all),
> > do the chmod() call from within move_temp_to_file().
>
> I think you would need this on top.

Agreed.

Thanks,

..Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH 2/2] Resolve double chmod() in move_temp_to_file()

[Johan Herland]

move_temp_to_file() used to chmod(foo, 0444) immediately before calling
adjust_shared_perm() which potentially does another call to chmod().

This patch splits a new function (called get_shared_perm()) out of
adjust_shared_perm(). The new function adjusts a given file mode value
according to the shared_repository setting, and returns the resulting
mode. It is used in move_temp_file() to generate the correct and final
permissions to pass to chmod() in a single call.

Signed-off-by: Johan Herland <johan@herland.net>
---
 cache.h     |    1 +
 path.c      |   26 +++++++++++++++++++-------
 sha1_file.c |    2 +-
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index 9cf5a13..b24eb3a 100644
--- a/cache.h
+++ b/cache.h
@@ -623,6 +623,7 @@ enum sharedrepo {
 	PERM_EVERYBODY      = 0664,
 };
 int git_config_perm(const char *var, const char *value);
+int get_shared_perm(int mode);
 int adjust_shared_perm(const char *path);
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
diff --git a/path.c b/path.c
index 42898e0..497db19 100644
--- a/path.c
+++ b/path.c
@@ -311,16 +311,13 @@ char *enter_repo(char *path, int strict)
 	return NULL;
 }
 
-int adjust_shared_perm(const char *path)
+int get_shared_perm(int mode)
 {
-	struct stat st;
-	int mode, tweak, shared;
+	int tweak, shared;
 
 	if (!shared_repository)
-		return 0;
-	if (lstat(path, &st) < 0)
-		return -1;
-	mode = st.st_mode;
+		return mode;
+
 	if (shared_repository < 0)
 		shared = -shared_repository;
 	else
@@ -343,6 +340,21 @@ int adjust_shared_perm(const char *path)
 		mode |= FORCE_DIR_SET_GID;
 	}
 
+	return mode;
+}
+
+int adjust_shared_perm(const char *path)
+{
+	struct stat st;
+	int mode, tweak, shared;
+
+	if (!shared_repository)
+		return 0;
+	if (lstat(path, &st) < 0)
+		return -1;
+
+	mode = get_shared_perm(st.st_mode);
+
 	if (((shared_repository < 0
 	      ? (st.st_mode & (FORCE_DIR_SET_GID | 0777))
 	      : (st.st_mode & mode)) != mode) &&
diff --git a/sha1_file.c b/sha1_file.c
index 87ac53b..05af3c5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2252,7 +2252,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 		/* FIXME!!! Collision check here ? */
 	}
 
-	if (chmod(filename, 0444) || adjust_shared_perm(filename))
+	if (chmod(filename, get_shared_perm(0444)))
 		return error("unable to set permission to '%s'", filename);
 	return 0;
 }
-- 
1.6.1.2.461.g5bad6

Re: [PATCH 2/2] Resolve double chmod() in move_temp_to_file()

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> diff --git a/sha1_file.c b/sha1_file.c
> index 87ac53b..05af3c5 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2252,7 +2252,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
>  		/* FIXME!!! Collision check here ? */
>  	}
>  
> -	if (chmod(filename, 0444) || adjust_shared_perm(filename))
> +	if (chmod(filename, get_shared_perm(0444)))

Your get_shared_perm() will end up feeding 0444 to S_ISDIR(), which would
most likely say "no" and cause real harm, but there is no guarantee that
we won't start checking S_ISREG() or other things in get_shared_perm()
later.  I do not like this.

How about doing it this way instead?

One thing to note is that we seem to have been passing what we read from
st.st_mode, together with S_IFMT bits, to chmod(2); I do not think I've
seen any breakage reports on exotic systems (glibc on Linux seems to
ignore the higher bits), but from my reading of POSIX, I would not be
surprised if somebody's chmod(2) returned EINVAL.

-- >8 --
set_shared_perm(): sometimes we know what the final mode bits should look like

adjust_shared_perm() first obtains the mode bits from lstat(2), expecting
to find what the result of applying user's umask is, and then tweaked it
as necessary.  When the file to be adjusted is created with mkstemp(3),
however, the mode thusly obtained does not have anything to do with usre's
umask, and we would need to start from 0444 in such a case and there is no
point running lstat(2) for such a path.

This introduces a new API set_shared_perm() to bypass the lstat(2) and
instead force setting the mode bits to the desired value directly.
adjust_shared_perm() becomes a thin wrapper to the function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     |    3 ++-
 path.c      |   25 ++++++++++++++++---------
 sha1_file.c |    4 ++--
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/cache.h b/cache.h
index 189151d..e283bbe 100644
--- a/cache.h
+++ b/cache.h
@@ -613,7 +613,8 @@ enum sharedrepo {
 	PERM_EVERYBODY      = 0664,
 };
 int git_config_perm(const char *var, const char *value);
-int adjust_shared_perm(const char *path);
+int set_shared_perm(const char *path, int mode);
+#define adjust_shared_perm(path) set_shared_perm((path), 0)
 int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 char *enter_repo(char *path, int strict);
diff --git a/path.c b/path.c
index 42898e0..8a0a674 100644
--- a/path.c
+++ b/path.c
@@ -311,16 +311,23 @@ char *enter_repo(char *path, int strict)
 	return NULL;
 }
 
-int adjust_shared_perm(const char *path)
+int set_shared_perm(const char *path, int mode)
 {
 	struct stat st;
-	int mode, tweak, shared;
+	int tweak, shared, orig_mode;
 
-	if (!shared_repository)
+	if (!shared_repository) {
+		if (mode)
+			return chmod(path, mode & ~S_IFMT);
 		return 0;
-	if (lstat(path, &st) < 0)
-		return -1;
-	mode = st.st_mode;
+	}
+	if (!mode) {
+		if (lstat(path, &st) < 0)
+			return -1;
+		mode = st.st_mode;
+		orig_mode = mode;
+	} else
+		orig_mode = 0;
 	if (shared_repository < 0)
 		shared = -shared_repository;
 	else
@@ -344,9 +351,9 @@ int adjust_shared_perm(const char *path)
 	}
 
 	if (((shared_repository < 0
-	      ? (st.st_mode & (FORCE_DIR_SET_GID | 0777))
-	      : (st.st_mode & mode)) != mode) &&
-	    chmod(path, mode) < 0)
+	      ? (orig_mode & (FORCE_DIR_SET_GID | 0777))
+	      : (orig_mode & mode)) != mode) &&
+	    chmod(path, (mode & ~S_IFMT)) < 0)
 		return -2;
 	return 0;
 }
diff --git a/sha1_file.c b/sha1_file.c
index 8869488..5bfc36c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2263,7 +2263,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 	 *
 	 * The same holds for FAT formatted media.
 	 *
-	 * When this succeeds, we just return 0. We have nothing
+	 * When this succeeds, we just return; we have nothing
 	 * left to unlink.
 	 */
 	if (ret && ret != EEXIST) {
@@ -2280,7 +2280,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 	}
 
 out:
-	if (chmod(filename, 0444) || adjust_shared_perm(filename))
+	if (set_shared_perm(filename, (S_IFREG|0444)))
 		return error("unable to set permission to '%s'", filename);
 	return 0;
 }

Re: [PATCH 2/2] Resolve double chmod() in move_temp_to_file()

[Johan Herland]

On Saturday 28 March 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > -	if (chmod(filename, 0444) || adjust_shared_perm(filename))
> > +	if (chmod(filename, get_shared_perm(0444)))
>
> Your get_shared_perm() will end up feeding 0444 to S_ISDIR(), which would
> most likely say "no" and cause real harm, but there is no guarantee that
> we won't start checking S_ISREG() or other things in get_shared_perm()
> later.  I do not like this.

You are right.

> How about doing it this way instead?
>
> One thing to note is that we seem to have been passing what we read from
> st.st_mode, together with S_IFMT bits, to chmod(2); I do not think I've
> seen any breakage reports on exotic systems (glibc on Linux seems to
> ignore the higher bits), but from my reading of POSIX, I would not be
> surprised if somebody's chmod(2) returned EINVAL.

Agreed.

> -- >8 --
> set_shared_perm(): sometimes we know what the final mode bits should look
> like
>
> adjust_shared_perm() first obtains the mode bits from lstat(2), expecting
> to find what the result of applying user's umask is, and then tweaked it

s/tweaked/tweaks/

> as necessary.  When the file to be adjusted is created with mkstemp(3),
> however, the mode thusly obtained does not have anything to do with
> usre's umask, and we would need to start from 0444 in such a case and

s/usre/user/

> there is no point running lstat(2) for such a path.
>
> This introduces a new API set_shared_perm() to bypass the lstat(2) and
> instead force setting the mode bits to the desired value directly.
> adjust_shared_perm() becomes a thin wrapper to the function.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

[...]

> diff --git a/sha1_file.c b/sha1_file.c
> index 8869488..5bfc36c 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2263,7 +2263,7 @@ int move_temp_to_file(const char *tmpfile, const
> char *filename) *
>  	 * The same holds for FAT formatted media.
>  	 *
> -	 * When this succeeds, we just return 0. We have nothing
> +	 * When this succeeds, we just return; we have nothing

Small nit: This belongs in the previous patch, doesn't it?


All in all, this looks very good. Please drop my second patch, and use this 
instead.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/2] Resolve double chmod() in move_temp_to_file()

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Saturday 28 March 2009, Junio C Hamano wrote:
>> Johan Herland <johan@herland.net> writes:
>> > -	if (chmod(filename, 0444) || adjust_shared_perm(filename))
>> > +	if (chmod(filename, get_shared_perm(0444)))
>>
>> Your get_shared_perm() will end up feeding 0444 to S_ISDIR(), which would
>> most likely say "no" and cause real harm, but there is no guarantee that
>> we won't start checking S_ISREG() or other things in get_shared_perm()
>> later.  I do not like this.

... should have been s/and cause/and cause no/; I think you read it correctly, though...

> ...
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 8869488..5bfc36c 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -2263,7 +2263,7 @@ int move_temp_to_file(const char *tmpfile, const
>> char *filename) *
>>  	 * The same holds for FAT formatted media.
>>  	 *
>> -	 * When this succeeds, we just return 0. We have nothing
>> +	 * When this succeeds, we just return; we have nothing
>
> Small nit: This belongs in the previous patch, doesn't it?

Thanks for all the above fixes.