When running git-maintenance(1), we create a lockfile that is supposed to keep other maintenance processes from running at the same time. This lockfile is broken though in case the "--detach" flag is passed: the lockfile is created by the parent process and will be cleaned up either manually or on exit. But when detaching, the parent will exit before all of the background maintenance tasks have been ran, and consequently the lock only covers a smaller part of the whole maintenance process.
Fix this bug by introducing two new functions:
- `daemonize_without_exit()` is the same as `daemonize()`, but doesn't
call exit(3p) for the parent process. - `lock_file_reassign_owner()` reassigns the owner of its owned
tempfiles so that they don't get unlinked anymore when the previous
owner exits.Together this allows us to reassign ownership of the lockfile after we have daemonized so that the lockfile is now owned by the child process.
Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com> Helped-by: Jeff King <peff@peff.net> Helped-by: Taylor Blau <me@ttaylorr.com> Helped-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/gc.c | 26 ++++++++++++++++++++-- lockfile.c | 9 ++++++++ lockfile.h | 10 +++++++++ setup.c | 31 +++++++++++++++++++-------- setup.h | 1 + t/t7900-maintenance.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 124 insertions(+), 11 deletions(-)
Show changes to 6 files +124 −11
builtin/gc.c, lockfile.c, lockfile.h, setup.c, setup.h, t/t7900-maintenance.sh
diff --git a/builtin/gc.c b/builtin/gc.c index 3a71e314c9..09cb92ac97 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts, TASK_PHASE_FOREGROUND)) result = 1; - /* Failure to daemonize is ok, we'll continue in foreground. */ if (opts->detach > 0) { + pid_t child_pid; + trace2_region_enter("maintenance", "detach", the_repository); - daemonize(); + + child_pid = daemonize_without_exit(); + if (!child_pid) { + /* + * We're in the child process, so we take ownership of + * the lockfile. + */ + lock_file_reassign_owner(&lk, getpid()); + } else if (child_pid > 0) { + /* + * We're in the parent process, so we assign ownership + * of the lockfile to the child and then exit immediately. + */ + lock_file_reassign_owner(&lk, child_pid); + exit(0); + } else { + /* + * Failure to daemonize is ok, we'll continue in + * foreground. + */ + } + trace2_region_leave("maintenance", "detach", the_repository); } diff --git a/lockfile.c b/lockfile.c index 7add2f136a..96aab3c885 100644 --- a/lockfile.c +++ b/lockfile.c @@ -356,3 +356,12 @@ int rollback_lock_file(struct lock_file *lk) delete_tempfile(&lk->pid_tempfile); return delete_tempfile(&lk->tempfile); } + +void lock_file_reassign_owner(struct lock_file *lk, pid_t owner) +{ + if (!is_lock_file_locked(lk)) + BUG("cannot reassign ownership of unlocked lockfile"); + lk->tempfile->owner = owner; + if (lk->pid_tempfile) + lk->pid_tempfile->owner = owner; +} diff --git a/lockfile.h b/lockfile.h index e7233f28de..0b10b624fa 100644 --- a/lockfile.h +++ b/lockfile.h @@ -341,4 +341,14 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path) */ int rollback_lock_file(struct lock_file *lk); +/* + * Reassign ownership of the lockfile to a different process. + * + * This is intended for use after `fork(2)`-ing. The parent transfers ownership + * to the daemonized child so that its atexit handler does not unlink the lock + * that should outlive it, and the child claims the inherited tempfiles so that + * they are cleaned up when the daemon exits. + */ +void lock_file_reassign_owner(struct lock_file *lk, pid_t owner); + #endif /* LOCKFILE_H */ diff --git a/setup.c b/setup.c index 7ec4427368..34deb6e985 100644 --- a/setup.c +++ b/setup.c @@ -2156,20 +2156,18 @@ void sanitize_stdfds(void) close(fd); } -int daemonize(void) +pid_t daemonize_without_exit(void) { #ifdef NO_POSIX_GOODIES errno = ENOSYS; return -1; #else - switch (fork()) { - case 0: - break; - case -1: - die_errno(_("fork failed")); - default: - exit(0); - } + pid_t pid = fork(); + if (pid < 0) + return -1; + if (pid > 0) + return pid; + if (setsid() == -1) die_errno(_("setsid failed")); close(0); @@ -2180,6 +2178,21 @@ int daemonize(void) #endif } +int daemonize(void) +{ +#ifdef NO_POSIX_GOODIES + errno = ENOSYS; + return -1; +#else + pid_t pid = daemonize_without_exit(); + if (pid < 0) + die_errno(_("fork failed")); + if (pid > 0) + exit(0); + return 0; +#endif +} + struct template_dir_cb_data { char *path; int initialized; diff --git a/setup.h b/setup.h index 80bc6e5f07..396af8d808 100644 --- a/setup.h +++ b/setup.h @@ -150,6 +150,7 @@ int path_inside_repo(const char *prefix, const char *path); void sanitize_stdfds(void); int daemonize(void); +pid_t daemonize_without_exit(void); /* * GIT_REPO_VERSION is the version we write by default. The diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 4700beacc1..df0bbc1669 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -1438,6 +1438,64 @@ test_expect_success '--no-detach causes maintenance to not run in background' ' ) ' +test_expect_success PIPE '--detach holds maintenance lock until daemonized child exits' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + + git config maintenance.auto false && + git config core.lockfilepid true && + + git remote add origin /does/not/exist && + git config set remote.origin.uploadpack "cat fifo-uploadpack" && + + mkfifo fifo-uploadpack fifo-maint && + + # Open the maintenance FIFO, as otherwise spawning + # git-maintenance(1) would block. Note that we need to open it + # as read-write, as otherwise we would block here already. + exec 9<>fifo-maint && + + { git maintenance run --task=prefetch --detach 7>&9 & } && + parent="$!" && + + # Reap the parent process so that the exec call below will not + # get SIGCHLD. + wait "$parent" && + + # Open the git-upload-pack(1) FIFO for writing, which will + # block until the upload-pack script opens it for reading. Once + # exec returns, we know that the daemonized child is alive and + # pinned. + exec 8>fifo-uploadpack && + + test_path_is_file .git/objects/maintenance.lock && + test_path_is_file .git/objects/"maintenance~pid.lock" && + + # Verify that the maintenance.lock still exists, and + # that it was created by the parent process, not the + # child. + echo "pid $parent" >expect && + test_cmp expect .git/objects/"maintenance~pid.lock" && + + # Reopen the maintenance FIFO as read-only so that + # git-maintenance(1) is the only writer. This will cause it to + # close the FIFO once the process exits. + exec 9<&- && + exec 9<fifo-maint && + + # Close the FIFO used by git-upload-pack(1) to unblock it and + # then wait until the maintenance FIFO is closed by + # git-maintenance(1), indicating that it has exited. + exec 8>&- && + cat <&9 && + + test_path_is_missing .git/objects/maintenance.lock && + test_path_is_missing .git/objects/"maintenance~pid.lock" + ) +' + test_expect_success '--detach causes maintenance to run in background' ' test_when_finished "rm -rf repo" && git init repo &&
-- 2.54.0.545.g6539524ca2.dirty