From: Jeff King <peff@peff.net>
Date: Wed, 21 Jan 2026 07:13:44 GMT
Subject: Re: [PATCH v5] lockfile: add PID file for debugging stale locks
Message-ID: <20260121071344.GA570838@coredump.intra.peff.net>
In-Reply-To: <pull.2011.v5.git.1768933954845.gitgitgadget@gmail.com>

On Tue, Jan 20, 2026 at 06:32:34PM +0000, Paulo Casaretto via GitGitGadget wrote:

> +static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
> +{
> +	struct strbuf content = STRBUF_INIT;
> +	struct tempfile *pid_tempfile = NULL;
> +	int fd = -1;
> +
> +	if (!lockfile_pid_enabled)
> +		goto out;
> +
> +	fd = open(pid_path, O_WRONLY | O_CREAT | O_EXCL, mode);
> +	if (fd < 0)
> +		goto out;
> +
> +	strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
> +	if (write_in_full(fd, content.buf, content.len) < 0) {
> +		warning_errno(_("could not write lock pid file '%s'"), pid_path);
> +		close(fd);
> +		fd = -1;
> +		unlink(pid_path);
> +		goto out;
> +	}
> +
> +	close(fd);
> +	fd = -1;
> +	pid_tempfile = register_tempfile(pid_path);
> +
> +out:
> +	if (fd >= 0)
> +		close(fd);
> +	strbuf_release(&content);
> +	return pid_tempfile;
> +}

Coverity complains that the close(fd) call in the "out" label is
unreachable, and I think it is right. When we jump from before the
open(), or if the open failed, then fd is negative (and thus no close).
If we get there when write_in_full() fails, then we close ourselves in
the conditional. And if we succeed, then we close the descriptor before
registering the tempfile.

I don't think it's wrong, but the cleanup is redundant between the "out"
path and the others.

Did you mean this:

diff --git a/lockfile.c b/lockfile.c
index 731cdd4944..e5d6ae0df6 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -122,14 +122,10 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
 	strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
 	if (write_in_full(fd, content.buf, content.len) < 0) {
 		warning_errno(_("could not write lock pid file '%s'"), pid_path);
-		close(fd);
-		fd = -1;
 		unlink(pid_path);
 		goto out;
 	}
 
-	close(fd);
-	fd = -1;
 	pid_tempfile = register_tempfile(pid_path);
 
 out:

which would just let the close after the out label handle all cases?

-Peff