patchssh signing: don't detach the filename strbuf from key_file tempfile

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

Signed-off-by: redoste <redoste@redoste.xyz>
---
 gpg-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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.

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:

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:

On 2025-07-05 at 19:21:13, Jeff King wrote:

I don't have a strong view either way, but I do wonder if it's a good idea to have the testsuite poking around in `/tmp`, although maybe if we honour `TMPDIR` then it would be possible to do in a tidy way.

I don't know if we have a strict policy. I do know that there are developers who always go by a pseudonym, such as chromatic[0], the contributor to Perl, and obviously we'd want to allow them to contribute. We also let people use shortened forms of their names or initials (for instance, Jeff King).

I also have some friends who are trans and have transitioned or are in the process of transitioning but have simply not gotten around to getting legal paperwork done[1]. Obviously they have a distinct and identifiable name that they go by and we'd allow them to use a preferred name.

There might also be good reasons that a contributor might not want to use a legal name: harassment, threats, employer hostility, fame[2], or a hostile government, to name a few. I think those are legitimate reasons to contribute pseudonymously.

So I would say that if someone has a distinct and identifiable identity that is pseudonymous and that is generally used and visible in the public sphere online, that's probably good enough. While I'm not a lawyer, it's my understanding that in many locales, making a legal promise of sorts (such as a sign-off) is equally binding whether made with one's real name or a pseudonym, so I don't see a problem with the legal aspect of it.

[0] https://en.wikipedia.org/wiki/Chromatic_(programmer) [1] In some locales this involves hiring an attorney, getting paperwork from a doctor, and getting a court order, so it can be expensive and kind of a hassle to do. It may also not be legally possible to do that in some places. [2] Notably the frontman of the band Weezer, Rivers Cuomo, is involved in coding under his real name (https://github.com/riverscuomo), but perhaps a CEO, musician, actor, or other famous person might not want their open-source contributions to be associated with their real name.

On Sat Jul 5, 2025 at 22:07 CEST, brian m. carlson wrote:

I looked into adding a test, but I didn't find any other tests checking for temporary files and I agree that messing in /tmp doesn't feel really appropriate for the testsuite.

Maybe something like this?

On Sun Jul 6, 2025 at 00:50 CEST, redoste wrote:

Sorry, it's my first time contributing via a mailing list and I stupidly copy-pasted the output of `git diff` without checking the whitespaces. Here is the proper version of the diff that can be applied:

On 2025-07-05 at 22:50:43, redoste wrote:

What I would recommend is adding another test (that is, another `test_expect_success` assertion) that when signing we don't leave any temporary files behind. You can then squash that into your patch and send a v2.

As for `ls`, I wouldn't use that in scripting. I'd use something like this:

---- mkdir tmpdir && TMPDIR="$(pwd)/tmpdir" && export TMPDIR && # Other code find tmpdir -type f >files && test_line_count = 0 files ----

I think `find` is typically better in scripting and also in this case with a separate directory as the temporary directory we'll find any left over temporary files, not just the pattern we've fixed here.

Our coding guidelines point out that some shells don't like `export` with assignments, so we do those as two lines. Normally I would write what you have in my day-to-day life, but we run on a variety of systems that most people will never use.

I think the basic premise is sound, but a separate assertion would make it easier to readers why we're setting the temporary directory.

No, I think this is fine. You weren't obligated to explain that and our policies should gracefully handle this situation regardless, but given what I said before and this context, I think the name you have is fine. Git unfortunately has poor support for replacing names and I would't want you to have to put your deadname into our history for all of eternity.

I'll send a patch to fix the policy.

On Sun, Jul 06, 2025 at 12:28:43AM +0000, brian m. carlson wrote:

Ah, I hadn't heard about that change in the kernel. I agree that it would make sense to loosen our text to match.

Thanks (in advance) for sending that doc update, and for your earlier comments in the thread. I agreed with everything you wrote there. FWIW, my personal inclinations lean much more towards allowing privacy and anonymity, but as a reviewer for the project I wanted to make sure we were not violating our own policy in a way that would eventually bite us in a legal sense. The notion of "identifiable but pseudonymous" you brought up seems like a good middle ground (at least to my layman's ears).

And thanks redoste for your patch. ;) Sorry if bringing up the signoff issue caused any hassle (but hopefully it will lead to an improvement to our docs).

-Peff

On Sun Jul 6, 2025 at 05:13 CEST, Jeff King wrote:

No worries! I completely understand your position as a reviewer and I don't mind if it helps to improve the situation and will help future contributors.

On Sun Jul 6, 2025 at 02:28 CEST, brian m. carlson wrote:

Thanks for the feedback!

I used `ls` since it was the easiest way to have something that fails quickly if a file doesn't exists (and `test -e` doesn't support having multiple files), but I agree, using `find` this way is definitely cleaner.

Thanks!

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

While it's possible to get rid of the allocation and copy from xstrdup(), it keeps the code symetric with the other branch since interpolate_path() also allocates and ssh_signing_key_file is freed in both cases.

Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: redoste <redoste@redoste.xyz>
---
v1->v2:
 * add a test case that checks for temporary files after signing commits
 * add small explaination about the use of xstrdup() in the commit
   body

 gpg-interface.c              |  2 +-
 t/t7528-signed-commit-ssh.sh | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

On 2025-07-06 at 17:34:49, redoste wrote:

Yup, this looks good to me. Thanks so much for the patch; I always appreciate keeping the temporary directory tidy.

On Sun, Jul 06, 2025 at 07:34:49PM +0200, redoste wrote:

I think this exported environment variable now leaks into subsequent tests, doesn't it? We may want to do it in a subshell.

	mkdir tmpdir &&
	TMPDIR="$(pwd)/tmpdir" &&
	(
		export TMPDIR &&
		ssh-add "${GPGSSH_KEY_PRIMARY}" &&
		echo 1 >file && git add file &&
		git commit -a -m inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
		echo 2 >file &&
		git commit -a -m file -S"${GPGSSH_KEY_PRIMARY}"
	) && 
	find tmpdir -type f >tmpfiles &&
	test_line_count = 0 tmpfiles

Patrick

On 07/07/2025 10:02, Patrick Steinhardt wrote:

That's a very good point.

The idiomatic way to test for an empty file is "test_file_is_empty <file>" which avoids spawning wc.

As this test is an abridged version of the other test that uses ssh-agent I think it would be more efficient to tweak that test to check there are no temporary files left over. Our test suite is slow and tweaking an existing test to improve our coverage instead of adding a new test means we don't make it any slower than necessary.

Thanks

Phillip

On Mon Jul 7, 2025 at 11:35 CEST, Phillip Wood wrote:

Sure thing, I took inspiration from the previous ssh-agent test and it exports the ssh-agent variables without spawning a subshell, so I figured it was fine, but it was only because test_when_finished will fail in a subshell. I guess it's probably fine to leak only the ssh-agent variables accross tests and keep TMPDIR in a subshell.

Thanks, I will update this.

I moved this test out of the other ssh-agent one according to the advice from brian on the previous version. I agree that spawning multiple ssh-agent is definitly not the best for speed, but is it worth it to make the test failure less clear? I feel like it's not the best to keep checks for temporary files clean up in a test that (initially) only claims to sign commits.

On 07/07/2025 15:25, redoste wrote:

Hopefully having the ssh-agent variables set after the agent has been killed doesn't affect the later tests

test_file_is_empty will print a diagnostic message if it fails so it should be clear what has caused the test failure

You can always tweak the test title if you want. The way I see it is that the changes that are being tested are related to commit signing as the invariant that we want to assert is that temporary files are cleaned up after signing commits.

Thanks

Phillip

On Mon Jul 7, 2025 at 17:26 CEST, Phillip Wood wrote:

Uh, okay, this makes sense and I think it will be clear enough. However there is only `test_file_not_empty`, `test_file_is_empty` doesn't exist. I will implement it in an other commit.

I see.

I will update this for a v3. Thanks for the feedback!

On 07/07/2025 17:22, redoste wrote:

Sorry I'd misremembered the name, it is "test_must_be_empty"

Phillip

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

While it's possible to get rid of the allocation and copy from xstrdup(), it keeps the code symetric with the other branch since interpolate_path() also allocates and ssh_signing_key_file is freed in both cases.

The exisiting test was updated to check if the temporary files are properly deleted. To prevent TMPDIR from leaking into the other tests, a new subshell is created, however this prevents test_config from working. The cleanup of the config changed in the subshell is done by test_unconfig in a call to test_when_finished outside of it.

Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: redoste <redoste@redoste.xyz>
---
v1->v2:
 * add a test case that checks for temporary files after signing commits
 * add small explaination about the use of xstrdup() in the commit
   body
v2->v3:
 * merge the test with the previous ssh-agent test
 * export TMPDIR in a subshell to prevent the environment variable from
   leaking in the next tests
 * use test_must_be_empty instead of test_line_count

 gpg-interface.c              |  2 +-
 t/t7528-signed-commit-ssh.sh | 32 ++++++++++++++++++++------------
 2 files changed, 21 insertions(+), 13 deletions(-)

redoste <redoste@redoste.xyz> writes:

These sound all good.  Will queue.

Thanks.

On Mon, Jul 07, 2025 at 01:57:39PM -0700, Junio C Hamano wrote:

Yup, the patch looks good to me. Thanks!

Patrick

On 08/07/2025 07:47, Patrick Steinhardt wrote:

Yes it does to me too, thanks

Phillip