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

[Junio C Hamano <gitster@pobox.com>]

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;
 }