[PATCH v2 00/22] Memory leak fixes (pt.4)

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 12399 bytes --]

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
-- 
2.46.0.46.g406f326d27.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]