| Message ID | cover.1723121979.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Memory leak fixes (pt.4) | expand |
Patrick Steinhardt <ps@pks.im> writes: > Hi, > > this is the second version of my fourth batch of patches that fix > various memory leaks. > > Changes compared to v1: > > - Adapt the memory leak fix for command characters to instead use a > `comment_line_str_allocated` variable. > > - Clarify some commit messages. > > - Drop the TODO comment about `rebase.gpgsign`. Turns out that this is > working as intended, as explained by Phillip. > > Thanks! > I went through the series and apart from some typos, everything looked great. I don't expect a reroll for those typos though, since they're minor. [snip]
Hi Patrick On 08/08/2024 14:04, Patrick Steinhardt wrote: > Hi, > > this is the second version of my fourth batch of patches that fix > various memory leaks. > > Changes compared to v1: > > - Adapt the memory leak fix for command characters to instead use a > `comment_line_str_allocated` variable. > > - Clarify some commit messages. > > - Drop the TODO comment about `rebase.gpgsign`. Turns out that this is > working as intended, as explained by Phillip. The changes to the rebase and sequencer patches look good to me Thanks Phillip > Thanks! > > Patrick > > Patrick Steinhardt (22): > remote: plug memory leak when aliasing URLs > git: fix leaking system paths > object-file: fix memory leak when reading corrupted headers > object-name: fix leaking symlink paths in object context > bulk-checkin: fix leaking state TODO > read-cache: fix leaking hashfile when writing index fails > submodule-config: fix leaking name enrty when traversing submodules > config: fix leaking comment character config > builtin/rebase: fix leaking `commit.gpgsign` value > builtin/notes: fix leaking `struct notes_tree` when merging notes > builtin/fast-import: plug trivial memory leaks > builtin/fast-export: fix leaking diff options > builtin/fast-export: plug leaking tag names > merge-ort: unconditionally release attributes index > sequencer: release todo list on error paths > unpack-trees: clear index when not propagating it > diff: fix leak when parsing invalid ignore regex option > builtin/format-patch: fix various trivial memory leaks > userdiff: fix leaking memory for configured diff drivers > builtin/log: fix leak when showing converted blob contents > diff: free state populated via options > builtin/diff: free symmetric diff members > > builtin/commit.c | 7 +- > builtin/diff.c | 10 ++- > builtin/fast-export.c | 19 ++++-- > builtin/fast-import.c | 8 ++- > builtin/log.c | 13 +++- > builtin/notes.c | 9 ++- > builtin/rebase.c | 1 + > bulk-checkin.c | 2 + > config.c | 4 +- > csum-file.c | 2 +- > csum-file.h | 10 +++ > diff.c | 16 ++++- > environment.c | 1 + > environment.h | 1 + > git.c | 12 +++- > merge-ort.c | 3 +- > object-file.c | 1 + > object-name.c | 1 + > range-diff.c | 6 +- > read-cache.c | 97 ++++++++++++++++----------- > remote.c | 2 + > sequencer.c | 67 ++++++++++++------ > submodule-config.c | 18 +++-- > t/t0210-trace2-normal.sh | 2 +- > t/t1006-cat-file.sh | 1 + > t/t1050-large.sh | 1 + > t/t1450-fsck.sh | 1 + > t/t1601-index-bogus.sh | 2 + > t/t2107-update-index-basic.sh | 1 + > t/t3310-notes-merge-manual-resolve.sh | 1 + > t/t3311-notes-merge-fanout.sh | 1 + > t/t3404-rebase-interactive.sh | 1 + > t/t3435-rebase-gpg-sign.sh | 1 + > t/t3507-cherry-pick-conflict.sh | 1 + > t/t3510-cherry-pick-sequence.sh | 1 + > t/t3705-add-sparse-checkout.sh | 1 + > t/t4013-diff-various.sh | 1 + > t/t4014-format-patch.sh | 1 + > t/t4018-diff-funcname.sh | 1 + > t/t4030-diff-textconv.sh | 2 + > t/t4042-diff-textconv-caching.sh | 2 + > t/t4048-diff-combined-binary.sh | 1 + > t/t4064-diff-oidfind.sh | 2 + > t/t4065-diff-anchored.sh | 1 + > t/t4068-diff-symmetric-merge-base.sh | 1 + > t/t4069-remerge-diff.sh | 1 + > t/t4108-apply-threeway.sh | 1 + > t/t4209-log-pickaxe.sh | 2 + > t/t6421-merge-partial-clone.sh | 1 + > t/t6428-merge-conflicts-sparse.sh | 1 + > t/t7008-filter-branch-null-sha1.sh | 1 + > t/t7030-verify-tag.sh | 1 + > t/t7817-grep-sparse-checkout.sh | 1 + > t/t9300-fast-import.sh | 1 + > t/t9304-fast-import-marks.sh | 2 + > t/t9351-fast-export-anonymize.sh | 1 + > unpack-trees.c | 2 + > userdiff.c | 38 ++++++++--- > userdiff.h | 4 ++ > 59 files changed, 288 insertions(+), 106 deletions(-) > > Range-diff against v1: > 1: 6e2fcd85c7 = 1: 2afa51f9ff remote: plug memory leak when aliasing URLs > 2: 9574995a24 = 2: 324140e4fd git: fix leaking system paths > 3: f7e67d02d2 = 3: 43a38a2281 object-file: fix memory leak when reading corrupted headers > 4: a9caaaed55 = 4: 9d3dc145e8 object-name: fix leaking symlink paths in object context > 5: 794af66103 = 5: 454139e7a4 bulk-checkin: fix leaking state TODO > 6: 2810cada0a = 6: f8b7195796 read-cache: fix leaking hashfile when writing index fails > 7: 03f699cf39 = 7: 762fb5aa73 submodule-config: fix leaking name enrty when traversing submodules > 8: a34c90a552 ! 8: 8fbd72a100 config: fix leaking comment character config > @@ Commit message > without free'ing the previous value. In fact, it can't easily free the > value in the first place because it may contain a string constant. > > - Refactor the code so that we initialize the value with another array. > - This allows us to free the value in case the string is not pointing to > - that constant array anymore. > + Refactor the code such that we track allocated comment character strings > + via a separate non-constant variable `comment_line_str_allocated`. Adapt > + sites that set `comment_line_str` to set both and free the old value > + that was stored in `comment_line_str_allocated`. > > This memory leak is being hit in t3404. As there are still other memory > leaks in that file we cannot yet mark it as passing with leak checking > @@ Commit message > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > + ## builtin/commit.c ## > +@@ builtin/commit.c: static void adjust_comment_line_char(const struct strbuf *sb) > + const char *p; > + > + if (!memchr(sb->buf, candidates[0], sb->len)) { > +- comment_line_str = xstrfmt("%c", candidates[0]); > ++ free(comment_line_str_allocated); > ++ comment_line_str = comment_line_str_allocated = > ++ xstrfmt("%c", candidates[0]); > + return; > + } > + > +@@ builtin/commit.c: static void adjust_comment_line_char(const struct strbuf *sb) > + if (!*p) > + die(_("unable to select a comment character that is not used\n" > + "in the current commit message")); > +- comment_line_str = xstrfmt("%c", *p); > ++ free(comment_line_str_allocated); > ++ comment_line_str = comment_line_str_allocated = xstrfmt("%c", *p); > + } > + > + static void prepare_amend_commit(struct commit *commit, struct strbuf *sb, > + > ## config.c ## > @@ config.c: static int git_default_core_config(const char *var, const char *value, > else if (value[0]) { > if (strchr(value, '\n')) > return error(_("%s cannot contain newline"), var); > -+ if (comment_line_str != comment_line_str_default) > -+ free((char *) comment_line_str); > - comment_line_str = xstrdup(value); > +- comment_line_str = xstrdup(value); > ++ free(comment_line_str_allocated); > ++ comment_line_str = comment_line_str_allocated = > ++ xstrdup(value); > auto_comment_line_char = 0; > } else > + return error(_("%s must have at least one character"), var); > > ## environment.c ## > @@ environment.c: int protect_ntfs = PROTECT_NTFS_DEFAULT; > - * The character that begins a commented line in user-editable file > * that is subject to stripspace. > */ > --const char *comment_line_str = "#"; > -+const char comment_line_str_default[] = "#"; > -+const char *comment_line_str = comment_line_str_default; > + const char *comment_line_str = "#"; > ++char *comment_line_str_allocated; > int auto_comment_line_char; > > /* Parallel index stat data preload? */ > > ## environment.h ## > @@ environment.h: struct strvec; > - * The character that begins a commented line in user-editable file > * that is subject to stripspace. > */ > -+extern const char comment_line_str_default[]; > extern const char *comment_line_str; > ++extern char *comment_line_str_allocated; > extern int auto_comment_line_char; > > + /* > 9: 05290fc1f1 ! 9: e497b76e9c builtin/rebase: fix leaking `commit.gpgsign` value > @@ Metadata > ## Commit message ## > builtin/rebase: fix leaking `commit.gpgsign` value > > - In `get_replay_opts()`, we unconditionally override the `gpg_sign` field > - that already got populated by `sequencer_init_config()` in case the user > - has "commit.gpgsign" set in their config. It is kind of dubious whether > - this is the correct thing to do or a bug. What is clear though is that > - this creates a memory leak. > + In `get_replay_opts()`, we override the `gpg_sign` field that already > + got populated by `sequencer_init_config()` in case the user has > + "commit.gpgsign" set in their config. This creates a memory leak because > + we overwrite the previously assigned value, which may have already > + pointed to an allocated string. > > - Let's mark this assignment with a TODO comment to figure out whether > - this needs to be fixed or not. Meanwhile though, let's plug the memory > - leak. > + Let's plug the memory leak by freeing the value before we overwrite it. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > @@ builtin/rebase.c: static struct replay_opts get_replay_opts(const struct rebase_ > replay.committer_date_is_author_date = > opts->committer_date_is_author_date; > replay.ignore_date = opts->ignore_date; > -+ > -+ /* > -+ * TODO: Is it really intentional that we unconditionally override > -+ * `replay.gpg_sign` even if it has already been initialized via the > -+ * configuration? > -+ */ > + free(replay.gpg_sign); > replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt); > -+ > replay.reflog_action = xstrdup(opts->reflog_action); > if (opts->strategy) > - replay.strategy = xstrdup_or_null(opts->strategy); > > ## sequencer.c ## > @@ sequencer.c: static int git_sequencer_config(const char *k, const char *v, > 10: 4f5d490074 = 10: c886b666f7 builtin/notes: fix leaking `struct notes_tree` when merging notes > 11: 798b911f77 = 11: d1c757157b builtin/fast-import: plug trivial memory leaks > 12: 660732d29d = 12: fa2d5c5d6b builtin/fast-export: fix leaking diff options > 13: 64366155de = 13: d9dd860d2a builtin/fast-export: plug leaking tag names > 14: b12015b3c3 = 14: 8f6860485e merge-ort: unconditionally release attributes index > 15: df4c21b49f ! 15: ea6a350f31 sequencer: release todo list on error paths > @@ sequencer.c: int sequencer_pick_revisions(struct repository *r, > &oid, > NULL); > - return error(_("%s: can't cherry-pick a %s"), > +- name, type_name(type)); > + res = error(_("%s: can't cherry-pick a %s"), > - name, type_name(type)); > ++ name, type_name(type)); > + goto out; > } > - } else > 16: 1f8553fd43 = 16: 2755023742 unpack-trees: clear index when not propagating it > 17: c6db8df324 = 17: edf6f148cd diff: fix leak when parsing invalid ignore regex option > 18: bf818a8a79 = 18: 343e3bd4df builtin/format-patch: fix various trivial memory leaks > 19: ef780aa360 = 19: be2c5b0bca userdiff: fix leaking memory for configured diff drivers > 20: f3882986a3 = 20: 7888203833 builtin/log: fix leak when showing converted blob contents > 21: a49bb2e0cc = 21: 245fc30afb diff: free state populated via options > 22: fb52599404 = 22: 343ddcd17b builtin/diff: free symmetric diff members
karthik nayak <karthik.188@gmail.com> writes: > I went through the series and apart from some typos, everything looked > great. I don't expect a reroll for those typos though, since they're > minor. Thanks for a review. A final reroll that shows only the typofixes in interdiff/range-diff is not a huge burden, but having to deal with many separate "here is a typo", "here is another typo" patches over a period is annoying and it is even worse that many readers have to get their reading distracted when seeing these leftover typoes, and get annoyed but yet not annoyed seriously enough to send these typofix patches to leave them unfixed to cause other readers' reading hiccupped. Let's always remember that there are 100x more readers than those of us who write. Thanks.
Phillip Wood <phillip.wood123@gmail.com> writes: > On 08/08/2024 14:04, Patrick Steinhardt wrote: >> Hi, >> this is the second version of my fourth batch of patches that fix >> various memory leaks. >> Changes compared to v1: >> - Adapt the memory leak fix for command characters to instead use >> a >> `comment_line_str_allocated` variable. >> - Clarify some commit messages. >> - Drop the TODO comment about `rebase.gpgsign`. Turns out that >> this is >> working as intended, as explained by Phillip. > > The changes to the rebase and sequencer patches look good to me > > Thanks > > Phillip Thanks for a review.
On Mon, Aug 12, 2024 at 08:49:59AM -0700, Junio C Hamano wrote: > karthik nayak <karthik.188@gmail.com> writes: > > > I went through the series and apart from some typos, everything looked > > great. I don't expect a reroll for those typos though, since they're > > minor. > > Thanks for a review. A final reroll that shows only the typofixes > in interdiff/range-diff is not a huge burden, but having to deal > with many separate "here is a typo", "here is another typo" patches > over a period is annoying and it is even worse that many readers > have to get their reading distracted when seeing these leftover > typoes, and get annoyed but yet not annoyed seriously enough to send > these typofix patches to leave them unfixed to cause other readers' > reading hiccupped. Let's always remember that there are 100x more > readers than those of us who write. Clue taken, I'll finally bite the bullet and set up spell correction in my editor. I'll also reroll this series later today. Patrick
Hi, this is the second version of my fourth batch of patches that fix various memory leaks. Changes compared to v1: - Adapt the memory leak fix for command characters to instead use a `comment_line_str_allocated` variable. - Clarify some commit messages. - Drop the TODO comment about `rebase.gpgsign`. Turns out that this is working as intended, as explained by Phillip. Thanks! Patrick Patrick Steinhardt (22): remote: plug memory leak when aliasing URLs git: fix leaking system paths object-file: fix memory leak when reading corrupted headers object-name: fix leaking symlink paths in object context bulk-checkin: fix leaking state TODO read-cache: fix leaking hashfile when writing index fails submodule-config: fix leaking name enrty when traversing submodules config: fix leaking comment character config builtin/rebase: fix leaking `commit.gpgsign` value builtin/notes: fix leaking `struct notes_tree` when merging notes builtin/fast-import: plug trivial memory leaks builtin/fast-export: fix leaking diff options builtin/fast-export: plug leaking tag names merge-ort: unconditionally release attributes index sequencer: release todo list on error paths unpack-trees: clear index when not propagating it diff: fix leak when parsing invalid ignore regex option builtin/format-patch: fix various trivial memory leaks userdiff: fix leaking memory for configured diff drivers builtin/log: fix leak when showing converted blob contents diff: free state populated via options builtin/diff: free symmetric diff members builtin/commit.c | 7 +- builtin/diff.c | 10 ++- builtin/fast-export.c | 19 ++++-- builtin/fast-import.c | 8 ++- builtin/log.c | 13 +++- builtin/notes.c | 9 ++- builtin/rebase.c | 1 + bulk-checkin.c | 2 + config.c | 4 +- csum-file.c | 2 +- csum-file.h | 10 +++ diff.c | 16 ++++- environment.c | 1 + environment.h | 1 + git.c | 12 +++- merge-ort.c | 3 +- object-file.c | 1 + object-name.c | 1 + range-diff.c | 6 +- read-cache.c | 97 ++++++++++++++++----------- remote.c | 2 + sequencer.c | 67 ++++++++++++------ submodule-config.c | 18 +++-- t/t0210-trace2-normal.sh | 2 +- t/t1006-cat-file.sh | 1 + t/t1050-large.sh | 1 + t/t1450-fsck.sh | 1 + t/t1601-index-bogus.sh | 2 + t/t2107-update-index-basic.sh | 1 + t/t3310-notes-merge-manual-resolve.sh | 1 + t/t3311-notes-merge-fanout.sh | 1 + t/t3404-rebase-interactive.sh | 1 + t/t3435-rebase-gpg-sign.sh | 1 + t/t3507-cherry-pick-conflict.sh | 1 + t/t3510-cherry-pick-sequence.sh | 1 + t/t3705-add-sparse-checkout.sh | 1 + t/t4013-diff-various.sh | 1 + t/t4014-format-patch.sh | 1 + t/t4018-diff-funcname.sh | 1 + t/t4030-diff-textconv.sh | 2 + t/t4042-diff-textconv-caching.sh | 2 + t/t4048-diff-combined-binary.sh | 1 + t/t4064-diff-oidfind.sh | 2 + t/t4065-diff-anchored.sh | 1 + t/t4068-diff-symmetric-merge-base.sh | 1 + t/t4069-remerge-diff.sh | 1 + t/t4108-apply-threeway.sh | 1 + t/t4209-log-pickaxe.sh | 2 + t/t6421-merge-partial-clone.sh | 1 + t/t6428-merge-conflicts-sparse.sh | 1 + t/t7008-filter-branch-null-sha1.sh | 1 + t/t7030-verify-tag.sh | 1 + t/t7817-grep-sparse-checkout.sh | 1 + t/t9300-fast-import.sh | 1 + t/t9304-fast-import-marks.sh | 2 + t/t9351-fast-export-anonymize.sh | 1 + unpack-trees.c | 2 + userdiff.c | 38 ++++++++--- userdiff.h | 4 ++ 59 files changed, 288 insertions(+), 106 deletions(-) Range-diff against v1: 1: 6e2fcd85c7 = 1: 2afa51f9ff remote: plug memory leak when aliasing URLs 2: 9574995a24 = 2: 324140e4fd git: fix leaking system paths 3: f7e67d02d2 = 3: 43a38a2281 object-file: fix memory leak when reading corrupted headers 4: a9caaaed55 = 4: 9d3dc145e8 object-name: fix leaking symlink paths in object context 5: 794af66103 = 5: 454139e7a4 bulk-checkin: fix leaking state TODO 6: 2810cada0a = 6: f8b7195796 read-cache: fix leaking hashfile when writing index fails 7: 03f699cf39 = 7: 762fb5aa73 submodule-config: fix leaking name enrty when traversing submodules 8: a34c90a552 ! 8: 8fbd72a100 config: fix leaking comment character config @@ Commit message without free'ing the previous value. In fact, it can't easily free the value in the first place because it may contain a string constant. - Refactor the code so that we initialize the value with another array. - This allows us to free the value in case the string is not pointing to - that constant array anymore. + Refactor the code such that we track allocated comment character strings + via a separate non-constant variable `comment_line_str_allocated`. Adapt + sites that set `comment_line_str` to set both and free the old value + that was stored in `comment_line_str_allocated`. This memory leak is being hit in t3404. As there are still other memory leaks in that file we cannot yet mark it as passing with leak checking @@ Commit message Signed-off-by: Patrick Steinhardt <ps@pks.im> + ## builtin/commit.c ## +@@ builtin/commit.c: static void adjust_comment_line_char(const struct strbuf *sb) + const char *p; + + if (!memchr(sb->buf, candidates[0], sb->len)) { +- comment_line_str = xstrfmt("%c", candidates[0]); ++ free(comment_line_str_allocated); ++ comment_line_str = comment_line_str_allocated = ++ xstrfmt("%c", candidates[0]); + return; + } + +@@ builtin/commit.c: static void adjust_comment_line_char(const struct strbuf *sb) + if (!*p) + die(_("unable to select a comment character that is not used\n" + "in the current commit message")); +- comment_line_str = xstrfmt("%c", *p); ++ free(comment_line_str_allocated); ++ comment_line_str = comment_line_str_allocated = xstrfmt("%c", *p); + } + + static void prepare_amend_commit(struct commit *commit, struct strbuf *sb, + ## config.c ## @@ config.c: static int git_default_core_config(const char *var, const char *value, else if (value[0]) { if (strchr(value, '\n')) return error(_("%s cannot contain newline"), var); -+ if (comment_line_str != comment_line_str_default) -+ free((char *) comment_line_str); - comment_line_str = xstrdup(value); +- comment_line_str = xstrdup(value); ++ free(comment_line_str_allocated); ++ comment_line_str = comment_line_str_allocated = ++ xstrdup(value); auto_comment_line_char = 0; } else + return error(_("%s must have at least one character"), var); ## environment.c ## @@ environment.c: int protect_ntfs = PROTECT_NTFS_DEFAULT; - * The character that begins a commented line in user-editable file * that is subject to stripspace. */ --const char *comment_line_str = "#"; -+const char comment_line_str_default[] = "#"; -+const char *comment_line_str = comment_line_str_default; + const char *comment_line_str = "#"; ++char *comment_line_str_allocated; int auto_comment_line_char; /* Parallel index stat data preload? */ ## environment.h ## @@ environment.h: struct strvec; - * The character that begins a commented line in user-editable file * that is subject to stripspace. */ -+extern const char comment_line_str_default[]; extern const char *comment_line_str; ++extern char *comment_line_str_allocated; extern int auto_comment_line_char; + /* 9: 05290fc1f1 ! 9: e497b76e9c builtin/rebase: fix leaking `commit.gpgsign` value @@ Metadata ## Commit message ## builtin/rebase: fix leaking `commit.gpgsign` value - In `get_replay_opts()`, we unconditionally override the `gpg_sign` field - that already got populated by `sequencer_init_config()` in case the user - has "commit.gpgsign" set in their config. It is kind of dubious whether - this is the correct thing to do or a bug. What is clear though is that - this creates a memory leak. + In `get_replay_opts()`, we override the `gpg_sign` field that already + got populated by `sequencer_init_config()` in case the user has + "commit.gpgsign" set in their config. This creates a memory leak because + we overwrite the previously assigned value, which may have already + pointed to an allocated string. - Let's mark this assignment with a TODO comment to figure out whether - this needs to be fixed or not. Meanwhile though, let's plug the memory - leak. + Let's plug the memory leak by freeing the value before we overwrite it. Signed-off-by: Patrick Steinhardt <ps@pks.im> @@ builtin/rebase.c: static struct replay_opts get_replay_opts(const struct rebase_ replay.committer_date_is_author_date = opts->committer_date_is_author_date; replay.ignore_date = opts->ignore_date; -+ -+ /* -+ * TODO: Is it really intentional that we unconditionally override -+ * `replay.gpg_sign` even if it has already been initialized via the -+ * configuration? -+ */ + free(replay.gpg_sign); replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt); -+ replay.reflog_action = xstrdup(opts->reflog_action); if (opts->strategy) - replay.strategy = xstrdup_or_null(opts->strategy); ## sequencer.c ## @@ sequencer.c: static int git_sequencer_config(const char *k, const char *v, 10: 4f5d490074 = 10: c886b666f7 builtin/notes: fix leaking `struct notes_tree` when merging notes 11: 798b911f77 = 11: d1c757157b builtin/fast-import: plug trivial memory leaks 12: 660732d29d = 12: fa2d5c5d6b builtin/fast-export: fix leaking diff options 13: 64366155de = 13: d9dd860d2a builtin/fast-export: plug leaking tag names 14: b12015b3c3 = 14: 8f6860485e merge-ort: unconditionally release attributes index 15: df4c21b49f ! 15: ea6a350f31 sequencer: release todo list on error paths @@ sequencer.c: int sequencer_pick_revisions(struct repository *r, &oid, NULL); - return error(_("%s: can't cherry-pick a %s"), +- name, type_name(type)); + res = error(_("%s: can't cherry-pick a %s"), - name, type_name(type)); ++ name, type_name(type)); + goto out; } - } else 16: 1f8553fd43 = 16: 2755023742 unpack-trees: clear index when not propagating it 17: c6db8df324 = 17: edf6f148cd diff: fix leak when parsing invalid ignore regex option 18: bf818a8a79 = 18: 343e3bd4df builtin/format-patch: fix various trivial memory leaks 19: ef780aa360 = 19: be2c5b0bca userdiff: fix leaking memory for configured diff drivers 20: f3882986a3 = 20: 7888203833 builtin/log: fix leak when showing converted blob contents 21: a49bb2e0cc = 21: 245fc30afb diff: free state populated via options 22: fb52599404 = 22: 343ddcd17b builtin/diff: free symmetric diff members