{"thread":{"id":"47482","subject":"[PATCH] sequencer: assign only free()able strings to gpg_sign","startedAt":"2017-12-22T11:51:06Z","lastAt":"2017-12-22T21:36:20Z","messageCount":5,"participants":["Johannes Schindelin","phillip.wood@talktalk.net","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"335166","messageId":"f4420880aa4ee73b7c8e435de1efccf9a969fd41.1513943347.git.johannes.schindelin@gmx.de","threadId":"47482","inReplyTo":null,"subject":"[PATCH] sequencer: assign only free()able strings to gpg_sign","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-22T11:50:50Z","receivedAt":"2017-12-22T11:51:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The gpg_sign member of the replay_opts structure is of type `char *`,\nmeaning that the sequencer deems the string to which gpg_sign points to\nbe under its custody, i.e. it needs to be free()d by the sequencer.\n\nTherefore, let's only assign malloc()ed buffers to it.\n\nReported-by: Kaartic Sivaraam \nSigned-off-by: Johannes Schindelin \n---\n\n\tPhillip, if you want to squash these changes into your patches,\n\tI'd totally fine with that.\n\nBased-On: pw/sequencer-in-process-commit at https://github.com/dscho/git\nFetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer-in-process-commit\nPublished-As: https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1\nFetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-gpg-sign-v1\n sequencer.c | 2 +-\n t/t3404-rebase-interactive.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7051b20b762..1b2599668f5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n \t}\n \n \tif (!strcmp(k, \"commit.gpgsign\")) {\n-\t\topts->gpg_sign = git_config_bool(k, v) ? \"\" : NULL;\n+\t\topts->gpg_sign = git_config_bool(k, v) ? xstrdup(\"\") : NULL;\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 9ed0a244e6c..040ef1a4dbc 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' '\n \n SQ=\"'\"\n test_expect_success 'rebase -i --gpg-sign=' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"edit 1\" git rebase -i --gpg-sign=\"\\\"S I Gner\\\"\" HEAD^ \\\n+\t\t>out 2>err &&\n+\ttest_i18ngrep \"$SQ-S\\\"S I Gner\\\"$SQ\" err\n+'\n+\n+test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\ttest_config commit.gpgsign true &&\n \tset_fake_editor &&\n \tFAKE_LINES=\"edit 1\" git rebase -i --gpg-sign=\"\\\"S I Gner\\\"\" HEAD^ \\\n \t\t>out 2>err &&\n\nbase-commit: 28d6daed4f119940ace31e523b3b272d3d153d04\n-- \n2.15.1.windows.2\n"},{"id":"335168","messageId":"17655681.1077671513947507373.JavaMail.defaultUser@defaultHost","threadId":"47482","inReplyTo":"f4420880aa4ee73b7c8e435de1efccf9a969fd41.1513943347.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] sequencer: assign only free()able strings to gpg_sign","fromName":"phillip.wood@talktalk.net","fromEmail":"phillip.wood@talktalk.net","sentAt":"2017-12-22T12:58:27Z","receivedAt":"2017-12-22T12:58:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"\n\n>----Original Message----\n>From: johannes.schindelin@gmx.de\n>Date: 22/12/2017 11:50 \n>To: \n>Cc: \"Junio C Hamano\", \"Phillip Wood\", \"Kaartic Sivaraam\"\n>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign\n>\n>The gpg_sign member of the replay_opts structure is of type `char *`,\n>meaning that the sequencer deems the string to which gpg_sign points \nto\n>be under its custody, i.e. it needs to be free()d by the sequencer.\n>\n>Therefore, let's only assign malloc()ed buffers to it.\n>\n>Reported-by: Kaartic Sivaraam \n>Signed-off-by: Johannes Schindelin \n>---\n>\n>\tPhillip, if you want to squash these changes into your patches,\n>\tI'd totally fine with that.\n>\n\nHi Johannes, thanks for putting this together, the patch it fixes is \nalready in next so I think it'd be best to leave this one separate. I \nwonder if it would be worth adding another test, see below.\n\nBest Wishes\n\n\nPhillip\n\n>Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git\n\n>Fetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer-\nin-process-commit\n>Published-As: https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1\n\n>Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-\ngpg-sign-v1\n> sequencer.c | 2 +-\n> t/t3404-rebase-interactive.sh | 10 ++++++++++\n> 2 files changed, 11 insertions(+), 1 deletion(-)\n>\n>diff --git a/sequencer.c b/sequencer.c\n>index 7051b20b762..1b2599668f5 100644\n>--- a/sequencer.c\n>+++ b/sequencer.c\n>@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, \nconst char *v, void *cb)\n> \t}\n> \n> \tif (!strcmp(k, \"commit.gpgsign\")) {\n>-\t\topts->gpg_sign = git_config_bool(k, v) ? \"\" : NULL;\n>+\t\topts->gpg_sign = git_config_bool(k, v) ? xstrdup(\"\") : NULL;\n> \t\treturn 0;\n> \t}\n> \n>diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-\ninteractive.sh\n>index 9ed0a244e6c..040ef1a4dbc 100755\n>--- a/t/t3404-rebase-interactive.sh\n>+++ b/t/t3404-rebase-interactive.sh\n>@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' '\n> \n> SQ=\"'\"\n> test_expect_success 'rebase -i --gpg-sign=' '\n>+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n>+\tset_fake_editor &&\n>+\tFAKE_LINES=\"edit 1\" git rebase -i --gpg-sign=\"\\\"S I Gner\\\"\" HEAD^ \\\n>+\t\t>out 2>err &&\n>+\ttest_i18ngrep \"$SQ-S\\\"S I Gner\\\"$SQ\" err\n>+'\n>+\n>+test_expect_success 'rebase -i --gpg-sign= overrides commit.\ngpgSign' '\n>+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n>+\ttest_config commit.gpgsign true &&\n> \tset_fake_editor &&\n> \tFAKE_LINES=\"edit 1\" git rebase -i --gpg-sign=\"\\\"S I Gner\\\"\" HEAD^ \\\n> \t\t>out 2>err &&\n\n\nI thought the bug could be triggered when commit.gpgsign was true and \nit was not overriden on the commandline, is it worth adding a test for \nthat?\n\n\n>base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04\n>-- \n>2.15.1.windows.2\n>\n\n\n"},{"id":"335174","messageId":"alpine.DEB.2.21.1.1712221817470.406@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47482","inReplyTo":"17655681.1077671513947507373.JavaMail.defaultUser@defaultHost","subject":"Re: [PATCH] sequencer: assign only free()able strings to gpg_sign","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-22T17:20:31Z","receivedAt":"2017-12-22T17:20:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 22 Dec 2017, phillip.wood@talktalk.net wrote:\n\n> I thought the bug could be triggered when commit.gpgsign was true and \n> it was not overriden on the commandline, is it worth adding a test for \n> that?\n\nEverybody working with extensive test suites seems to write/blog these\ndays that you have to be careful to test meaningfully, i.e. you need to\navoid making running the test suite so expensive that developers start to\navoid running it.\n\nIn that light, what do we want to test? If we want to verify that the\ngpg_sign is correctly allocated before it is free()d, then the test case I\nadded *already* covers it, and another test case would only increase the\nruntime of the test suite (which, as I hinted above, I deem a bad thing).\n\nSo I'm really in favor of keeping the tests concise.\n\nCiao,\nDscho\n"},{"id":"335180","messageId":"xmqqpo76sj15.fsf@gitster.mtv.corp.google.com","threadId":"47482","inReplyTo":"alpine.DEB.2.21.1.1712221817470.406@MININT-6BKU6QN.europe.corp.microsoft.com","subject":"Re: [PATCH] sequencer: assign only free()able strings to gpg_sign","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T18:40:38Z","receivedAt":"2017-12-22T18:40:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> I thought the bug could be triggered when commit.gpgsign was true and \n>> it was not overriden on the commandline, is it worth adding a test for \n>> that?\n>\n> ... If we want to verify that the\n> gpg_sign is correctly allocated before it is free()d, then the test case I\n> added *already* covers it,...\n\nIt depends on what we are testing, how we anticipate this code will\nbe broken by others in the future and how we want to futureproof the\ncode. We can say \"already covers it\" if we know the implementation\n(especially, the code calls free() when it replaces opts->gpg_sign\nalways, so the other side you are choosing not to test will die the\nsame way) and assume it won't be broken (i.e. the attitude is OK for\nwhitebox testing).\n\nI'd think that this particular case it is sufficient to test just\none; I doubt that adding another will increse the test load\nmeasurably, though.\n\n\n\n\n\n"},{"id":"335198","messageId":"xmqqr2rmphrq.fsf@gitster.mtv.corp.google.com","threadId":"47482","inReplyTo":"17655681.1077671513947507373.JavaMail.defaultUser@defaultHost","subject":"Re: [PATCH] sequencer: assign only free()able strings to gpg_sign","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T21:36:09Z","receivedAt":"2017-12-22T21:36:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"phillip.wood@talktalk.net\" writes:\n\n>>----Original Message----\n>>From: johannes.schindelin@gmx.de\n>>Date: 22/12/2017 11:50 \n>>To: \n>>Cc: \"Junio C Hamano\", \"Phillip Wood\" wood@dunelm.org.uk>, \"Kaartic Sivaraam\"\n>>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign\n>>\n>>The gpg_sign member of the replay_opts structure is of type `char *`,\n>>meaning that the sequencer deems the string to which gpg_sign points \n> to\n>>be under its custody, i.e. it needs to be free()d by the sequencer.\n>>\n>>Therefore, let's only assign malloc()ed buffers to it.\n>>\n>>Reported-by: Kaartic Sivaraam \n>>Signed-off-by: Johannes Schindelin \n>>---\n>>\n>>\tPhillip, if you want to squash these changes into your patches,\n>>\tI'd totally fine with that.\n>>\n>\n> Hi Johannes, thanks for putting this together, the patch it fixes is \n> already in next so I think it'd be best to leave this one separate. I \n> wonder if it would be worth adding another test, see below.\n\nThanks, both. Let's queue this on top as a fix-up.\n\n"}]}