"GOT", but the "O" is a cute, smiling sun Index | Thread

From:
Omar Polo <op@omarpolo.com>
Subject:
attempt at respecting umask
To:
gameoftrees@openbsd.org
Cc:
Date:
Thu, 27 Oct 2022 20:16:28 +0200

Download raw body.

This is a (sad) attempt at making `got update' follow umask(2).

Under most circumstances we shouldn't do any extra work: when
checking out file open(2) does the work for us; the issue lays when
updating files.

In those cases, we need to either set or clear the xbit, but we
can't directly do that.  When updating we're using a fd provided
by mkstemp (via got_opentemp_named_fd) and it's likely to be 0600...
I assume that's why after open(2) we do an fchmod(2) to reset the
permission.  Loosing umask tho!

Diff below moves the fchmod(2) only in the update case and does an
(IMHO ugly) umask dance to compute the correct value.  Includes a
matching test case; existing tests are passing.

Suggestions are welcome!

diff /home/op/w/got
commit - ad4cc36168576274131539e87b9007ef9b3c3725
path + /home/op/w/got
blob - d0e5af7c33c6ca8a4ba2e9dc621a494ed26dda57
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -1124,7 +1124,7 @@ get_ondisk_perms(int executable, mode_t st_mode)
 		return st_mode | xbits;
 	}
 
-	return (st_mode & ~(S_IXUSR | S_IXGRP | S_IXOTH));
+	return st_mode;
 }
 
 /* forward declaration */
@@ -1388,9 +1388,11 @@ install_blob(struct got_worktree *worktree, const char
 	size_t len, hdrlen;
 	int update = 0;
 	char *tmppath = NULL;
+	mode_t um, mode;
 
+	mode = get_ondisk_perms(te_mode & S_IXUSR, GOT_DEFAULT_FILE_MODE);
 	fd = open(ondisk_path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW |
-	    O_CLOEXEC, GOT_DEFAULT_FILE_MODE);
+	    O_CLOEXEC, mode);
 	if (fd == -1) {
 		if (errno == ENOENT) {
 			char *parent;
@@ -1403,7 +1405,7 @@ install_blob(struct got_worktree *worktree, const char
 				return err;
 			fd = open(ondisk_path,
 			    O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC,
-			    GOT_DEFAULT_FILE_MODE);
+			    mode);
 			if (fd == -1)
 				return got_error_from_errno2("open",
 				    ondisk_path);
@@ -1425,17 +1427,24 @@ install_blob(struct got_worktree *worktree, const char
 				if (err)
 					goto done;
 				update = 1;
+
+				/*
+				 * umask dance because we can't set the
+				 * base permissions for mkstemp.
+				 */
+				um = umask(000);
+				umask(um);
+				mode = mode & ~um;
+				if (fchmod(fd, mode) == -1) {
+					err = got_error_from_errno2("fchmod",
+					    tmppath);
+					goto done;
+				}
 			}
 		} else
 			return got_error_from_errno2("open", ondisk_path);
 	}
 
-	if (fchmod(fd, get_ondisk_perms(te_mode & S_IXUSR, st_mode)) == -1) {
-		err = got_error_from_errno2("fchmod",
-		    update ? tmppath : ondisk_path);
-		goto done;
-	}
-
 	if (progress_cb) {
 		if (restoring_missing_file)
 			err = (*progress_cb)(progress_arg, GOT_STATUS_MISSING,
blob - fce60c7f277d948ec481a744ed23e1b55ec128ed
file + regress/cmdline/update.sh
--- regress/cmdline/update.sh
+++ regress/cmdline/update.sh
@@ -3010,6 +3010,70 @@ test_parseargs "$@"
 	test_done "$testroot" "0"
 }
 
+test_update_umask() {
+	local testroot=`test_init update_binary_file`
+
+	got checkout "$testroot/repo" "$testroot/wt" >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	rm "$testroot/wt/alpha"
+
+	# using a subshell to avoid clobbering global umask
+	(umask 022 && cd "$testroot/wt" && got update alpha) \
+		>/dev/null 2>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-rw-r--r--; then
+		echo "alpha is not 0644" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	rm "$testroot/wt/alpha"
+
+	# using a subshell to avoid clobbering global umask
+	(umask 044 && cd "$testroot/wt" && got update alpha) \
+		>/dev/null 2>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-rw-------; then
+		echo "alpha is not 0600" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	rm "$testroot/wt/alpha"
+
+	# using a subshell to avoid clobbering global umask
+	(umask 222 && cd "$testroot/wt" && got update alpha) \
+		>/dev/null 2>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-r--r--r--; then
+		echo "alpha is not 0444" >&2
+		test_done "$testroot" 1
+		return 1;
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_update_basic
 run_test test_update_adds_file
@@ -3054,3 +3118,4 @@ run_test test_update_binary_file
 run_test test_update_file_skipped_due_to_obstruction
 run_test test_update_quiet
 run_test test_update_binary_file
+run_test test_update_umask