From 5a688fe4706462dfa0a7932ef0c82c335c47e9ab Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 25 Mar 2009 16:19:36 -0700 Subject: [PATCH 1/5] "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 ways that all 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 --- 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 ee3911f8eef5c0..8199e5d4d51688 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 e332b504a6b8c6..42898e0fb129d2 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 6c2deda18492ac..ebd60de9ce5b52 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 a07aa4e5c491d1..45987bdea8aadb 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 653362ba221ee0..d459854e71e1a5 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 From fb8b193670b0c11d118185332efc899d6d01d5f4 Mon Sep 17 00:00:00 2001 From: Johan Herland Date: Thu, 26 Mar 2009 16:16:47 +0100 Subject: [PATCH 2/5] Move chmod(foo, 0444) into move_temp_to_file() 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 Signed-off-by: Junio C Hamano --- 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 3748ddf48d9bde..d5fc042bbfe970 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 30d2d340418f7f..968b6b0662a89d 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 0dbad3c888c6c9..c5a3ea3b31045b 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 7fee8725333860..5dfe03ee6cbbf5 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 45987bdea8aadb..3bd20e715b785a 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2279,7 +2279,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; } @@ -2305,7 +2305,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)); } From 3be1f18e1b15c28ac6c750ff1a42576fd981d0f5 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Mar 2009 23:14:39 -0700 Subject: [PATCH 3/5] 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 --- sha1_file.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 3bd20e715b785a..6f278593e5ecf6 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2263,12 +2263,12 @@ 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) { 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; From 17e61b82887fb71800b0fcd39ffe89ddf4d2492e Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 27 Mar 2009 23:21:00 -0700 Subject: [PATCH 4/5] 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 tweaks 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 user'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 --- cache.h | 3 ++- path.c | 25 ++++++++++++++++--------- sha1_file.c | 2 +- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/cache.h b/cache.h index 189151de25ffd1..e283bbe1736ad7 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 42898e0fb129d2..8a0a6741fd664f 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 6f278593e5ecf6..d978abf43d26a4 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -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; } From 1b89eaa4bef44ef84f2af611d5db8727e3be266c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 31 Mar 2009 16:36:00 -0400 Subject: [PATCH 5/5] t1301: loosen test for forced modes One of the aspects of the test checked explicitly for the g+s bit to be set on created directories. However, this is only the means to an end (the "end" being having the correct group set). And in fact, on systems where DIR_HAS_BSD_GROUP_SEMANTICS is set, we do not even need to use this "means" at all, causing the test to fail. This patch removes that part of the test. In an ideal world it would be replaced by a test to check that the group was properly assigned, but that is difficult to automate because it requires the user running the test suite be a member of multiple groups. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1301-shared-repo.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index d459854e71e1a5..3c8a2373ac0bf8 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -147,8 +147,8 @@ test_expect_success 'forced modes' ' # 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)" && + # All directories must have either 2770 or 770 + test -z "$(sed -n -e "/^drwxrw[sx]---/d" -e "/^d/p" actual)" && # post-update hook must be 0770 test -z "$(sed -n -e "/post-update/{