Show 246 quoted lines
> 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