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@xxxxxxxxxxx>
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:
> 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
