From: Omar Polo Subject: attempt at respecting umask To: gameoftrees@openbsd.org Cc: Date: Thu, 27 Oct 2022 20:16:28 +0200 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