builtin/maintenance: fix locking with "--detach"

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 run, and consequently the
lock only covers a smaller part of the whole maintenance process.

Fix this bug by reassigning all tempfiles from the parent process to the
child process when daemonizing so that it becomes the responsibility of
the child to clean them up.

Note that this is a broader fix, as we now always reassign tempfiles
when daemonizing. This is a natural consequence of the semantics of
`daemonize()` though, as it essentially promises to continue running the
current process in the background. It is thus sensible to have that
function perform the whole dance of assigning resources to the child
process, including tempfiles.

There's only a single other caller in "daemon.c", but that process
doesn't create any tempfiles before the call to `daemonize()` and is
thus not impacted by this change.

Reported-by: Jean-Christophe Manciot <actionmystique@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Derrick Stolee <stolee@gmail.com>
Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/setup.c b/setup.c
index 7ec4427368a2a756dc7a45905209a08d12cffaba..14445a71a4297ecb591b18b3129b76c0f89e48a1 100644 (file)
--- a/setup.c
+++ b/setup.c
@@ -2162,12 +2162,26 @@ int daemonize(void)
        errno = ENOSYS;
        return -1;
 #else
-       switch (fork()) {
+       pid_t parent_pid = getpid();
+       pid_t child_pid = fork();
+
+       switch (child_pid) {
                case 0:
+                       /*
+                        * We're in the child process, so we take ownership of
+                        * all tempfiles.
+                        */
+                       reassign_tempfile_ownership(parent_pid, getpid());
                        break;
                case -1:
                        die_errno(_("fork failed"));
                default:
+                       /*
+                        * We're in the parent process, so we drop ownership of
+                        * all tempfiles to prevent us from removing them upon
+                        * exit.
+                        */
+                       reassign_tempfile_ownership(parent_pid, child_pid);
                        exit(0);
        }
        if (setsid() == -1)

diff --git a/setup.h b/setup.h
index 80bc6e5f078af8a94aa2488be90d0201ace2f305..b5bc5f280c34f09e107727ca4c146c25e34ffec8 100644 (file)
--- a/setup.h
+++ b/setup.h
@@ -149,6 +149,21 @@ void verify_non_filename(const char *prefix, const char *name);
 int path_inside_repo(const char *prefix, const char *path);
 
 void sanitize_stdfds(void);
+
+/*
+ * Daemonize the current process by forking and then exiting the parent
+ * process. Returns 0 when successful, in which case the parent process will
+ * have exited and it's the child process that continues to run the code.
+ * Otherwise, a negative error code is returned and the parent process will
+ * continue execution.
+ *
+ * Note that this function will also perform the following changes:
+ *
+ *   - Standard file descriptors in the child process are closed.
+ *   - The child process is made a session leader via setsid(3p).
+ *   - All tempfiles owned by the parent process are reassigned to the
+ *     daemonized child process.
+ */
 int daemonize(void);
 
 /*

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4700beacc18281413fc8d88ff35ea6ce392b0208..df0bbc166929874abb895762d0d6949f5e80356a 100755 (executable)
--- 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 &&

diff --git a/tempfile.c b/tempfile.c
index 82dfa3d82f2ac9c842088d6833181cd6214b56a9..f0fdf582794ba590d485099634b814e88848537f 100644 (file)
--- a/tempfile.c
+++ b/tempfile.c
@@ -373,3 +373,15 @@ int delete_tempfile(struct tempfile **tempfile_p)
 
        return err ? -1 : 0;
 }
+
+void reassign_tempfile_ownership(pid_t from, pid_t to)
+{
+       volatile struct volatile_list_head *pos;
+
+       list_for_each(pos, &tempfile_list) {
+               struct tempfile *p = list_entry(pos, struct tempfile, list);
+
+               if (is_tempfile_active(p) && p->owner == from)
+                       p->owner = to;
+       }
+}

diff --git a/tempfile.h b/tempfile.h
index 2d2ae5b657d4a97a7fe7b8e2e84c2062717fdde5..2227a095fd42898ab49643cc51ddbe950b821073 100644 (file)
--- a/tempfile.h
+++ b/tempfile.h
@@ -282,4 +282,15 @@ int delete_tempfile(struct tempfile **tempfile_p);
  */
 int rename_tempfile(struct tempfile **tempfile_p, const char *path);
 
+/*
+ * Reassign ownership of all active tempfiles whose `owner` field matches
+ * `from` to `to`.
+ *
+ * This is intended for use by `daemonize()`; after `fork(2)`-ing, the parent
+ * transfers ownership to the daemonized child so that its atexit handler does
+ * not unlink tempfiles that should outlive it, and the child claims the
+ * inherited tempfiles so that they are cleaned up when the daemon exits.
+ */
+void reassign_tempfile_ownership(pid_t from, pid_t to);
+
 #endif /* TEMPFILE_H */