Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

From: Jeff King <peff@peff.net>
Date: Jul 5, 2025, 19:21 UTC
Message-ID: <20250705192113.GB2496172@coredump.intra.peff.net>
In-Reply-To: <20250704230829.29696-1-redoste@redoste.xyz>

On Sat, Jul 05, 2025 at 01:08:28AM +0200, redoste wrote:

> Detaching the filename string from the tempfile structure used to cause
> delete_tempfile() to fail and the temporary file was not cleaned up.

Good catch. I can reproduce this easily with:

  git -c gpg.format=ssh \
      -c user.signingkey=key::does-not-exist \
      commit --allow-empty -S -m foo

which creates /tmp/.git_signing_key_tmp* and never cleans it up.

I wonder if it is worth adding a test, or if it would be too weirdly focused on this obscure case to be very useful against future regressions.

> Signed-off-by: redoste <redoste@redoste.xyz>

We look for a real name in the sign-off trailer, since it indicates an acceptance of the DCO and the ability to legally contribute the patch to the project. See the section of Documentation/SubmittingPatches starting with the '[[dco]]'. Or here:

  https://git-scm.com/docs/SubmittingPatches#sign-off

Looking at your web page, it looks like you may prefer not to associate your online identity with a legal name. I can't remember if we've dealt with this before. I'm adding brian to the cc, who has given a lot of thought to naming and privacy issues.

(In this particular case, I suspect the patch is trivial enough not to even be copyright-able, so we may be able to just let it go in this instance.)

The patch itself looks good:

Show 13 quoted lines

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 0896458de5..bdcc8c2a2e 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
>  				    key_file->filename.buf);
>  			goto out;
>  		}
> -		ssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);
> +		ssh_signing_key_file = xstrdup(key_file->filename.buf);
>  	} else {
>  		/* We assume a file */
>  		ssh_signing_key_file = interpolate_path(signing_key, 1);

I think we could avoid even xstrdup() here and just use the key_file pointer directly (it lasts until the end of the function, which is all we need). But the call to interpolate_path() on the other side of conditional does always allocate, so we'd need extra book-keeping to decide whether to free or not. The extra copy is worth it to keep the code simpler.

One other thing I noticed while looking at this function: the tempfile deletion code should be OK with a NULL entry (that is considered an "inactive" tempfile). So:

diff --git a/gpg-interface.c b/gpg-interface.c
index 0896458de5..fc48f93e11 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1102,10 +1102,8 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 	remove_cr_after(signature, bottom);
 
 out:
-	if (key_file)
-		delete_tempfile(&key_file);
-	if (buffer_file)
-		delete_tempfile(&buffer_file);
+	delete_tempfile(&key_file);
+	delete_tempfile(&buffer_file);
 	if (ssh_signature_filename.len)
 		unlink_or_warn(ssh_signature_filename.buf);
 	strbuf_release(&signer_stderr);

would simplify the code a little bit. But it may not be worth worrying
too much about (and is certainly orthogonal to your patch).

-Peff