{"thread":{"id":"56417","subject":"[PATCH] fast-export: fix anonymized tag using original length","startedAt":"2021-08-31T15:55:59Z","lastAt":"2021-08-31T19:35:04Z","messageCount":3,"participants":["Tal Kelrich via GitGitGadget","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"434293","messageId":"pull.1030.git.1630425354496.gitgitgadget@gmail.com","threadId":"56417","inReplyTo":null,"subject":"[PATCH] fast-export: fix anonymized tag using original length","fromName":"Tal Kelrich via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T15:55:54Z","receivedAt":"2021-08-31T15:55:59Z","isPatch":true,"sender":{"key":"hasturkun@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10245?v=4"},"body":"From: Tal Kelrich \n\nCommit 7f4075949686 (fast-export: tighten anonymize_mem() interface to\nhandle only strings, 2020-06-23) changed the interface used in anonymizing\nstrings, but failed to update the size of annotated tag messages to match\nthe new anonymized string.\n\nAs a result, exporting tags having messages longer than 13 characters\nwould create output that couldn't be parsed by fast-import,\nas the data length indicated was larger than the data output.\n\nReset the message size when anonymizing, and add a tag with a \"long\"\nmessage to the test.\n\nSigned-off-by: Tal Kelrich \n---\n fast-export: fix anonymized tag using original length\n \n Fixes an issue with fast-export anonymization, where any original\n annotated tag message longer than 13 characters would make the output\n unimportable by fast-import (as the data length indicated the original\n length).\n \n This also resolves a more minor issue, where if the original message was\n short, the anonymized tag message would be truncated.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1030%2Fhasturkun%2Ffast-export-tag-length-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1030/hasturkun/fast-export-tag-length-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1030\n\n builtin/fast-export.c | 1 +\n t/t9351-fast-export-anonymize.sh | 10 ++++++----\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 3c20f164f0f..95e8e89e81f 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -821,6 +821,7 @@ static void handle_tag(const char *name, struct tag *tag)\n \t\t\tstatic struct hashmap tags;\n \t\t\tmessage = anonymize_str(&tags, anonymize_tag,\n \t\t\t\t\t\tmessage, message_size, NULL);\n+\t\t\tmessage_size = strlen(message);\n \t\t}\n \t}\n \ndiff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh\nindex 1c6e6fcdaf3..77047e250dc 100755\n--- a/t/t9351-fast-export-anonymize.sh\n+++ b/t/t9351-fast-export-anonymize.sh\n@@ -18,7 +18,8 @@ test_expect_success 'setup simple repo' '\n \tgit update-index --add --cacheinfo 160000,$fake_commit,link1 &&\n \tgit update-index --add --cacheinfo 160000,$fake_commit,link2 &&\n \tgit commit -m \"add gitlink\" &&\n-\tgit tag -m \"annotated tag\" mytag\n+\tgit tag -m \"annotated tag\" mytag &&\n+\tgit tag -m \"annotated tag with long message\" longtag\n '\n \n test_expect_success 'export anonymized stream' '\n@@ -55,7 +56,8 @@ test_expect_success 'stream retains other as refname' '\n \n test_expect_success 'stream omits other refnames' '\n \t! grep main stream &&\n-\t! grep mytag stream\n+\t! grep mytag stream &&\n+\t! grep longtag stream\n '\n \n test_expect_success 'stream omits identities' '\n@@ -118,9 +120,9 @@ test_expect_success 'identical gitlinks got identical oid' '\n \ttest_line_count = 1 commits\n '\n \n-test_expect_success 'tag points to branch tip' '\n+test_expect_success 'all tags point to branch tip' '\n \tgit rev-parse $other_branch >expect &&\n-\tgit for-each-ref --format=\"%(*objectname)\" | grep . >actual &&\n+\tgit for-each-ref --format=\"%(*objectname)\" | grep . | uniq >actual &&\n \ttest_cmp expect actual\n '\n \n\nbase-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee\n-- \ngitgitgadget\n"},{"id":"434321","messageId":"xmqqlf4htoqa.fsf@gitster.g","threadId":"56417","inReplyTo":"pull.1030.git.1630425354496.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-export: fix anonymized tag using original length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-31T19:10:05Z","receivedAt":"2021-08-31T19:10:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tal Kelrich via GitGitGadget\" writes:\n\n> From: Tal Kelrich \n>\n> Commit 7f4075949686 (fast-export: tighten anonymize_mem() interface to\n> handle only strings, 2020-06-23) changed the interface used in anonymizing\n> strings, but failed to update the size of annotated tag messages to match\n> the new anonymized string.\n>\n> As a result, exporting tags having messages longer than 13 characters\n> would create output that couldn't be parsed by fast-import,\n> as the data length indicated was larger than the data output.\n>\n> Reset the message size when anonymizing, and add a tag with a \"long\"\n> message to the test.\n>\n> Signed-off-by: Tal Kelrich \n> ---\n> fast-export: fix anonymized tag using original length\n> \n> Fixes an issue with fast-export anonymization, where any original\n> annotated tag message longer than 13 characters would make the output\n> unimportable by fast-import (as the data length indicated the original\n> length).\n> \n> This also resolves a more minor issue, where if the original message was\n> short, the anonymized tag message would be truncated.\n\nThanks. I've looked at all places where anonymize_str() is used and\nfound that this is the only problematic caller.\n\nWill queue.\n"},{"id":"434324","messageId":"YS6EZKXCxohA7Wt8@coredump.intra.peff.net","threadId":"56417","inReplyTo":"pull.1030.git.1630425354496.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-export: fix anonymized tag using original length","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-31T19:35:00Z","receivedAt":"2021-08-31T19:35:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 03:55:54PM +0000, Tal Kelrich via GitGitGadget wrote:\n\n> From: Tal Kelrich \n> \n> Commit 7f4075949686 (fast-export: tighten anonymize_mem() interface to\n> handle only strings, 2020-06-23) changed the interface used in anonymizing\n> strings, but failed to update the size of annotated tag messages to match\n> the new anonymized string.\n> \n> As a result, exporting tags having messages longer than 13 characters\n> would create output that couldn't be parsed by fast-import,\n> as the data length indicated was larger than the data output.\n> \n> Reset the message size when anonymizing, and add a tag with a \"long\"\n> message to the test.\n\nThanks, good catch and nicely explained. I wondered where the \"13\" came\nfrom, and it is \"tag message %d\" (so really, it depends how many tags\nyou have :) ).\n\n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index 3c20f164f0f..95e8e89e81f 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -821,6 +821,7 @@ static void handle_tag(const char *name, struct tag *tag)\n> \t\t\tstatic struct hashmap tags;\n> \t\t\tmessage = anonymize_str(&tags, anonymize_tag,\n> \t\t\t\t\t\tmessage, message_size, NULL);\n> +\t\t\tmessage_size = strlen(message);\n> \t\t}\n> \t}\n\nIn the other callers, we just treat the return value from\nanonymize_str() like a string afterwards, so they naturally adapted to\nthe anonymized content. But here the rest of the tag code uses the size,\nso we have to update it. Makes sense (both why the fix here, but why a\nsimilar fix is not needed elsewhere).\n\n> diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh\n> index 1c6e6fcdaf3..77047e250dc 100755\n> --- a/t/t9351-fast-export-anonymize.sh\n> +++ b/t/t9351-fast-export-anonymize.sh\n> @@ -18,7 +18,8 @@ test_expect_success 'setup simple repo' '\n> \tgit update-index --add --cacheinfo 160000,$fake_commit,link1 &&\n> \tgit update-index --add --cacheinfo 160000,$fake_commit,link2 &&\n> \tgit commit -m \"add gitlink\" &&\n> -\tgit tag -m \"annotated tag\" mytag\n> +\tgit tag -m \"annotated tag\" mytag &&\n> +\tgit tag -m \"annotated tag with long message\" longtag\n> '\n\nI was curious why the existing tests did not catch even the truncation.\nI think we only assert that the original tag content does not appear.\nBut moreover, the values before/after anonymization coincidentally have\nexactly the same length:\n\n annotated tag\n tag message 1\n\nso it did not even trigger the bug.\n\n-Peff\n"}]}