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@xxxxxx>
+ ## 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@xxxxxx>
@@ 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
--
2.46.0.46.g406f326d27.dirty
Attachment:
signature.asc
Description: PGP signature
