threads / patch / 47482

patchsequencer: assign only free()able strings to gpg_sign

Subject: [PATCH] sequencer: assign only free()able strings to gpg_sign

## tl;dr

5 messages between Dec 22, 2017 and Dec 22, 2017. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Johannes Schindelin· Dec 22, 2017, 11:50 UTC · lore

The gpg_sign member of the replay_opts structure is of type `char *`, meaning that the sequencer deems the string to which gpg_sign points to be under its custody, i.e. it needs to be free()d by the sequencer.

Therefore, let's only assign malloc()ed buffers to it.
Reported-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	Phillip, if you want to squash these changes into your patches,
	I'd totally fine with that.
Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer-in-process-commit
Published-As: https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1
Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-gpg-sign-v1
 sequencer.c                   |  2 +-
 t/t3404-rebase-interactive.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)
Show changes to 2 files +11 −1

sequencer.c, t/t3404-rebase-interactive.sh

diff --git a/sequencer.c b/sequencer.c
index 7051b20b762..1b2599668f5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
 	}
 
 	if (!strcmp(k, "commit.gpgsign")) {
-		opts->gpg_sign = git_config_bool(k, v) ? "" : NULL;
+		opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL;
 		return 0;
 	}
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9ed0a244e6c..040ef1a4dbc 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' '
 
 SQ="'"
 test_expect_success 'rebase -i --gpg-sign=<key-id>' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
+		>out 2>err &&
+	test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
+'
+
+test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.gpgSign' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	test_config commit.gpgsign true &&
 	set_fake_editor &&
 	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
 		>out 2>err &&

base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04

-- 
2.15.1.windows.2
phillip.wood@talktalk.net· Dec 22, 2017, 12:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

Show 5 quoted lines
>----Original Message----
>From: johannes.schindelin@gmx.de
>Date: 22/12/2017 11:50 
>To: <git@vger.kernel.org>
>Cc: "Junio C Hamano"<gitster@pobox.com>, "Phillip Wood"<phillip.
wood@dunelm.org.uk>, "Kaartic Sivaraam"<kaartic.sivaraam@gmail.com>
>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign
>
>The gpg_sign member of the replay_opts structure is of type `char *`,
>meaning that the sequencer deems the string to which gpg_sign points 
to
Show 11 quoted lines
>be under its custody, i.e. it needs to be free()d by the sequencer.
>
>Therefore, let's only assign malloc()ed buffers to it.
>
>Reported-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
>Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>---
>
>	Phillip, if you want to squash these changes into your patches,
>	I'd totally fine with that.
>

Hi Johannes, thanks for putting this together, the patch it fixes is already in next so I think it'd be best to leave this one separate. I wonder if it would be worth adding another test, see below.

Best Wishes
Phillip
>Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git
>Fetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer-
in-process-commit
>Published-As: https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1
>Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-
gpg-sign-v1
Show 9 quoted lines
> sequencer.c                   |  2 +-
> t/t3404-rebase-interactive.sh | 10 ++++++++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/sequencer.c b/sequencer.c
>index 7051b20b762..1b2599668f5 100644
>--- a/sequencer.c
>+++ b/sequencer.c
>@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, 
const char *v, void *cb)
Show 9 quoted lines
> 	}
> 
> 	if (!strcmp(k, "commit.gpgsign")) {
>-		opts->gpg_sign = git_config_bool(k, v) ? "" : NULL;
>+		opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL;
> 		return 0;
> 	}
> 
>diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-
interactive.sh
Show 15 quoted lines
>index 9ed0a244e6c..040ef1a4dbc 100755
>--- a/t/t3404-rebase-interactive.sh
>+++ b/t/t3404-rebase-interactive.sh
>@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' '
> 
> SQ="'"
> test_expect_success 'rebase -i --gpg-sign=<key-id>' '
>+	test_when_finished "test_might_fail git rebase --abort" &&
>+	set_fake_editor &&
>+	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
>+		>out 2>err &&
>+	test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
>+'
>+
>+test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.
gpgSign' '
Show 5 quoted lines
>+	test_when_finished "test_might_fail git rebase --abort" &&
>+	test_config commit.gpgsign true &&
> 	set_fake_editor &&
> 	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
> 		>out 2>err &&

I thought the bug could be triggered when commit.gpgsign was true and it was not overriden on the commandline, is it worth adding a test for that?

>base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04
>-- 
>2.15.1.windows.2
>
Johannes Schindelin· Dec 22, 2017, 17:20 UTC · re: phillip.wood@talktalk.net · lore

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

Hi Phillip,
On Fri, 22 Dec 2017, phillip.wood@talktalk.net wrote:
> I thought the bug could be triggered when commit.gpgsign was true and 
> it was not overriden on the commandline, is it worth adding a test for 
> that?

Everybody working with extensive test suites seems to write/blog these days that you have to be careful to test meaningfully, i.e. you need to avoid making running the test suite so expensive that developers start to avoid running it.

In that light, what do we want to test? If we want to verify that the gpg_sign is correctly allocated before it is free()d, then the test case I added *already* covers it, and another test case would only increase the runtime of the test suite (which, as I hinted above, I deem a bad thing).

So I'm really in favor of keeping the tests concise.

Ciao, Dscho

Junio C Hamano· Dec 22, 2017, 18:40 UTC · re: Johannes Schindelin · lore

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
>> I thought the bug could be triggered when commit.gpgsign was true and 
>> it was not overriden on the commandline, is it worth adding a test for 
>> that?
>
> ... If we want to verify that the
> gpg_sign is correctly allocated before it is free()d, then the test case I
> added *already* covers it,...

It depends on what we are testing, how we anticipate this code will be broken by others in the future and how we want to futureproof the code. We can say "already covers it" if we know the implementation (especially, the code calls free() when it replaces opts->gpg_sign always, so the other side you are choosing not to test will die the same way) and assume it won't be broken (i.e. the attitude is OK for whitebox testing).

I'd think that this particular case it is sufficient to test just one; I doubt that adding another will increse the test load measurably, though.

Junio C Hamano· Dec 22, 2017, 21:36 UTC · re: phillip.wood@talktalk.net · lore

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

"phillip.wood@talktalk.net" <phillip.wood@talktalk.net> writes:
Show 26 quoted lines
>>----Original Message----
>>From: johannes.schindelin@gmx.de
>>Date: 22/12/2017 11:50 
>>To: <git@vger.kernel.org>
>>Cc: "Junio C Hamano"<gitster@pobox.com>, "Phillip Wood"<phillip.
> wood@dunelm.org.uk>, "Kaartic Sivaraam"<kaartic.sivaraam@gmail.com>
>>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign
>>
>>The gpg_sign member of the replay_opts structure is of type `char *`,
>>meaning that the sequencer deems the string to which gpg_sign points 
> to
>>be under its custody, i.e. it needs to be free()d by the sequencer.
>>
>>Therefore, let's only assign malloc()ed buffers to it.
>>
>>Reported-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
>>Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>---
>>
>>	Phillip, if you want to squash these changes into your patches,
>>	I'd totally fine with that.
>>
>
> Hi Johannes, thanks for putting this together, the patch it fixes is 
> already in next so I think it'd be best to leave this one separate. I 
> wonder if it would be worth adding another test, see below.
Thanks, both.  Let's queue this on top as a fix-up.

← back to recent threads