Re: [PATCH] fast-export: fix anonymized tag using original length
- From
Jeff King <peff@peff.net>
- Date
- Aug 31, 2021, 19:35 UTC
- Message-ID
- <YS6EZKXCxohA7Wt8@coredump.intra.peff.net>
- In-Reply-To
- <pull.1030.git.1630425354496.gitgitgadget@gmail.com>
On Tue, Aug 31, 2021 at 03:55:54PM +0000, Tal Kelrich via GitGitGadget wrote:
Show 13 quoted lines
> From: Tal Kelrich <hasturkun@gmail.com> > > Commit 7f4075949686 (fast-export: tighten anonymize_mem() interface to > handle only strings, 2020-06-23) changed the interface used in anonymizing > strings, but failed to update the size of annotated tag messages to match > the new anonymized string. > > As a result, exporting tags having messages longer than 13 characters > would create output that couldn't be parsed by fast-import, > as the data length indicated was larger than the data output. > > Reset the message size when anonymizing, and add a tag with a "long" > message to the test.
Thanks, good catch and nicely explained. I wondered where the "13" came from, and it is "tag message %d" (so really, it depends how many tags you have :) ).
Show 11 quoted lines
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c > index 3c20f164f0f..95e8e89e81f 100644 > --- a/builtin/fast-export.c > +++ b/builtin/fast-export.c > @@ -821,6 +821,7 @@ static void handle_tag(const char *name, struct tag *tag) > static struct hashmap tags; > message = anonymize_str(&tags, anonymize_tag, > message, message_size, NULL); > + message_size = strlen(message); > } > }
In the other callers, we just treat the return value from anonymize_str() like a string afterwards, so they naturally adapted to the anonymized content. But here the rest of the tag code uses the size, so we have to update it. Makes sense (both why the fix here, but why a similar fix is not needed elsewhere).
Show 12 quoted lines
> diff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh > index 1c6e6fcdaf3..77047e250dc 100755 > --- a/t/t9351-fast-export-anonymize.sh > +++ b/t/t9351-fast-export-anonymize.sh > @@ -18,7 +18,8 @@ test_expect_success 'setup simple repo' ' > git update-index --add --cacheinfo 160000,$fake_commit,link1 && > git update-index --add --cacheinfo 160000,$fake_commit,link2 && > git commit -m "add gitlink" && > - git tag -m "annotated tag" mytag > + git tag -m "annotated tag" mytag && > + git tag -m "annotated tag with long message" longtag > '
I was curious why the existing tests did not catch even the truncation. I think we only assert that the original tag content does not appear. But moreover, the values before/after anonymization coincidentally have exactly the same length:
annotated tag tag message 1
so it did not even trigger the bug.
-Peff