[PATCH v3 00/21] Various memory leak fixes

[Patrick Steinhardt <ps@pks.im>]

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

Hi,

this is the third version of my patch series that fixes various memory
leaks throughout Git.

Changes compared to v2:

    - Add a missing SOB.

    - Fix some typos in commit messages.

    - Explain more thoroughly why `appendwholeline()` may leak even on
      error.

    - Move removal of `const char **` cast to the correct commit.

Thanks!

Patrick

Patrick Steinhardt (21):
  ci: add missing dependency for TTY prereq
  t: mark a bunch of tests as leak-free
  transport-helper: fix leaking helper name
  strbuf: fix leak when `appendwholeline()` fails with EOF
  checkout: clarify memory ownership in `unique_tracking_name()`
  http: refactor code to clarify memory ownership
  config: clarify memory ownership in `git_config_pathname()`
  diff: refactor code to clarify memory ownership of prefixes
  convert: refactor code to clarify ownership of
    check_roundtrip_encoding
  builtin/log: stop using globals for log config
  builtin/log: stop using globals for format config
  config: clarify memory ownership in `git_config_string()`
  config: plug various memory leaks
  builtin/credential: clear credential before exit
  commit-reach: fix memory leak in `ahead_behind()`
  submodule: fix leaking memory for submodule entries
  strvec: add functions to replace and remove strings
  builtin/mv: refactor `add_slash()` to always return allocated strings
  builtin/mv duplicate string list memory
  builtin/mv: refactor to use `struct strvec`
  builtin/mv: fix leaks for submodule gitfile paths

 Makefile                                      |   1 +
 alias.c                                       |   6 +-
 attr.c                                        |   2 +-
 attr.h                                        |   2 +-
 builtin/blame.c                               |   2 +-
 builtin/checkout.c                            |  14 +-
 builtin/commit.c                              |   4 +-
 builtin/config.c                              |   2 +-
 builtin/credential.c                          |   2 +
 builtin/log.c                                 | 708 ++++++++++--------
 builtin/merge.c                               |   4 +-
 builtin/mv.c                                  | 222 +++---
 builtin/rebase.c                              |   2 +-
 builtin/receive-pack.c                        |   6 +-
 builtin/repack.c                              |   8 +-
 builtin/worktree.c                            |  20 +-
 checkout.c                                    |   4 +-
 checkout.h                                    |   6 +-
 ci/install-dependencies.sh                    |   4 +-
 commit-reach.c                                |   4 +
 config.c                                      |  52 +-
 config.h                                      |  10 +-
 convert.c                                     |  30 +-
 convert.h                                     |   2 +-
 delta-islands.c                               |   2 +-
 diff.c                                        |  20 +-
 environment.c                                 |  16 +-
 environment.h                                 |  14 +-
 fetch-pack.c                                  |   4 +-
 fsck.c                                        |   4 +-
 fsmonitor-settings.c                          |   5 +-
 gpg-interface.c                               |   6 +-
 http.c                                        |  50 +-
 imap-send.c                                   |  12 +-
 mailmap.c                                     |   4 +-
 mailmap.h                                     |   4 +-
 merge-ll.c                                    |   6 +-
 pager.c                                       |   2 +-
 pretty.c                                      |  14 +-
 promisor-remote.h                             |   2 +-
 remote.c                                      |  20 +-
 remote.h                                      |   8 +-
 sequencer.c                                   |   2 +-
 setup.c                                       |   6 +-
 strbuf.c                                      |   4 +-
 strvec.c                                      |  20 +
 strvec.h                                      |  13 +
 submodule-config.c                            |   2 +
 t/t0300-credentials.sh                        |   2 +
 t/t0411-clone-from-partial.sh                 |   1 +
 t/t0610-reftable-basics.sh                    |   1 +
 t/t0611-reftable-httpd.sh                     |   1 +
 t/t1013-read-tree-submodule.sh                |   1 +
 t/t1306-xdg-files.sh                          |   1 +
 t/t1350-config-hooks-path.sh                  |   1 +
 t/t1400-update-ref.sh                         |   2 +
 t/t2013-checkout-submodule.sh                 |   1 +
 t/t2024-checkout-dwim.sh                      |   1 +
 t/t2060-switch.sh                             |   1 +
 t/t2405-worktree-submodule.sh                 |   1 +
 t/t3007-ls-files-recurse-submodules.sh        |   1 +
 t/t3203-branch-output.sh                      |   2 +
 t/t3415-rebase-autosquash.sh                  |   1 +
 t/t3426-rebase-submodule.sh                   |   1 +
 t/t3512-cherry-pick-submodule.sh              |   1 +
 t/t3513-revert-submodule.sh                   |   1 +
 t/t3600-rm.sh                                 |   1 +
 t/t3906-stash-submodule.sh                    |   1 +
 t/t4001-diff-rename.sh                        |   4 +-
 t/t4041-diff-submodule-option.sh              |   1 +
 t/t4043-diff-rename-binary.sh                 |   1 +
 t/t4059-diff-submodule-not-initialized.sh     |   1 +
 t/t4060-diff-submodule-option-diff-format.sh  |   1 +
 t/t4120-apply-popt.sh                         |   1 +
 t/t4137-apply-submodule.sh                    |   1 +
 t/t4210-log-i18n.sh                           |   2 +
 t/t5563-simple-http-auth.sh                   |   1 +
 t/t5564-http-proxy.sh                         |   1 +
 t/t5581-http-curl-verbose.sh                  |   1 +
 t/t6006-rev-list-format.sh                    |   1 +
 t/t6041-bisect-submodule.sh                   |   1 +
 t/t6400-merge-df.sh                           |   1 +
 t/t6412-merge-large-rename.sh                 |   1 +
 t/t6426-merge-skip-unneeded-updates.sh        |   1 +
 t/t6429-merge-sequence-rename-caching.sh      |   1 +
 t/t6438-submodule-directory-file-conflicts.sh |   1 +
 t/t7001-mv.sh                                 |   2 +
 t/t7005-editor.sh                             |   1 +
 t/t7102-reset.sh                              |   1 +
 t/t7112-reset-submodule.sh                    |   1 +
 t/t7417-submodule-path-url.sh                 |   1 +
 t/t7421-submodule-summary-add.sh              |   1 +
 t/t7423-submodule-symlinks.sh                 |   1 +
 t/t9129-git-svn-i18n-commitencoding.sh        |   1 -
 t/t9139-git-svn-non-utf8-commitencoding.sh    |   1 -
 t/t9200-git-cvsexportcommit.sh                |   1 +
 t/t9401-git-cvsserver-crlf.sh                 |   1 +
 t/t9600-cvsimport.sh                          |   1 +
 t/t9601-cvsimport-vendor-branch.sh            |   1 +
 t/t9602-cvsimport-branches-tags.sh            |   1 +
 t/t9603-cvsimport-patchsets.sh                |   2 +
 t/t9604-cvsimport-timestamps.sh               |   2 +
 t/unit-tests/t-strvec.c                       | 269 +++++++
 t/unit-tests/test-lib.c                       |  13 +
 t/unit-tests/test-lib.h                       |  13 +
 transport-helper.c                            |   6 +-
 transport.c                                   |   1 +
 upload-pack.c                                 |   2 +-
 userdiff.h                                    |  12 +-
 109 files changed, 1151 insertions(+), 586 deletions(-)
 create mode 100644 t/unit-tests/t-strvec.c

Range-diff against v2:
 1:  857b8b14ce !  1:  ded220a06b ci: add missing dependency for TTY prereq
    @@ Commit message
         systems are missing this dependency and thus don't execute those tests
         at all. Fix this.
     
    +    Signed-off-by: Patrick Steinhardt <ps@pks.im>
    +
      ## ci/install-dependencies.sh ##
     @@ ci/install-dependencies.sh: alpine-*)
      	apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
 2:  ceade7dbba !  2:  f305e4bc46 t: mark a bunch of tests as leak-free
    @@ Commit message
           - t7423: Introduced via b20c10fd9b (t7423: add tests for symlinked
             submodule directories, 2024-01-28), passes since e8d0608944
             (submodule: require the submodule path to contain directories only,
    -        2024-03-26). The fix is not ovbiously related, but probably works
    +        2024-03-26). The fix is not obviously related, but probably works
             because we now die early in many code paths.
     
           - t9xxx: All of these are exercising CVS-related tooling and pass
             since at least Git v2.40. It's likely that these pass for a long
    -        time already, but nobody ever noticed because noone has CVS on their
    -        machine.
    +        time already, but nobody ever noticed because Git developers do not
    +        tend to have CVS on their machines.
     
         Mark all of these tests as passing.
     
 3:  a96b5ac359 =  3:  9e946c1a83 transport-helper: fix leaking helper name
 4:  9dd8709d1b !  4:  aa5cbd9d14 strbuf: fix leak when `appendwholeline()` fails with EOF
    @@ Commit message
     
         In `strbuf_appendwholeline()` we call `strbuf_getwholeline()` with a
         temporary buffer. In case the call returns an error we indicate this by
    -    returning EOF, but never release the temporary buffer. This can lead to
    -    a memory leak when the line has been partially filled. Fix this.
    +    returning EOF, but never release the temporary buffer. This can cause a
    +    leak though because `strbuf_getwholeline()` calls getline(3). Quoting
    +    its documentation:
    +
    +        If *lineptr was set to NULL before the call, then the buffer
    +        should be freed by the user program even on failure.
    +
    +    Consequently, the temporary buffer may hold allocated memory even when
    +    the call to `strbuf_getwholeline()` fails.
    +
    +    Fix this by releasing the temporary buffer on error.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
 5:  6d4e9ce706 =  5:  4a6dd9b6a8 checkout: clarify memory ownership in `unique_tracking_name()`
 6:  141cae2de1 =  6:  fa91a3942e http: refactor code to clarify memory ownership
 7:  ff5e761e55 =  7:  88babf1abf config: clarify memory ownership in `git_config_pathname()`
 8:  afe69c7303 =  8:  feec7e971f diff: refactor code to clarify memory ownership of prefixes
 9:  eb7fce55b0 =  9:  dae00f1b63 convert: refactor code to clarify ownership of check_roundtrip_encoding
10:  ee2fcf388c = 10:  02c5be27be builtin/log: stop using globals for log config
11:  3490ad3a02 = 11:  eeba79678a builtin/log: stop using globals for format config
12:  6cfc28c7e2 ! 12:  a4fafcd079 config: clarify memory ownership in `git_config_string()`
    @@ config.c: int git_config_bool(const char *name, const char *value)
      {
      	if (!value)
      		return config_error_nonbool(var);
    +@@ config.c: static int git_default_core_config(const char *var, const char *value,
    + 
    + 	if (!strcmp(var, "core.checkroundtripencoding")) {
    + 		FREE_AND_NULL(check_roundtrip_encoding);
    +-		return git_config_string((const char **) &check_roundtrip_encoding, var, value);
    ++		return git_config_string(&check_roundtrip_encoding, var, value);
    + 	}
    + 
    + 	if (!strcmp(var, "core.notesref")) {
     @@ config.c: int git_configset_get_string(struct config_set *set, const char *key, char **des
      {
      	const char *value;
13:  70e8e26513 ! 13:  8b74dff678 config: plug various memory leaks
    @@ config.c: static int git_default_core_config(const char *var, const char *value,
      		return git_config_pathname(&git_hooks_path, var, value);
      	}
      
    -@@ config.c: static int git_default_core_config(const char *var, const char *value,
    - 
    - 	if (!strcmp(var, "core.checkroundtripencoding")) {
    - 		FREE_AND_NULL(check_roundtrip_encoding);
    --		return git_config_string((const char **) &check_roundtrip_encoding, var, value);
    -+		return git_config_string(&check_roundtrip_encoding, var, value);
    - 	}
    - 
    - 	if (!strcmp(var, "core.notesref")) {
     @@ config.c: static int git_default_core_config(const char *var, const char *value,
      		return 0;
      	}
14:  f1a1c43e76 = 14:  265665fe6c builtin/credential: clear credential before exit
15:  64b92156f8 = 15:  b315a5bb5c commit-reach: fix memory leak in `ahead_behind()`
16:  cd8a992f08 = 16:  7c75a94756 submodule: fix leaking memory for submodule entries
17:  128e2eaf7a = 17:  0f61ee9929 strvec: add functions to replace and remove strings
18:  1310b24fc2 = 18:  1830e2a568 builtin/mv: refactor `add_slash()` to always return allocated strings
19:  d4fef9825a = 19:  9eeafac365 builtin/mv duplicate string list memory
20:  797cdb286a = 20:  48b9d3e343 builtin/mv: refactor to use `struct strvec`
21:  095469193c = 21:  add7946446 builtin/mv: fix leaks for submodule gitfile paths
-- 
2.45.1.246.gb9cfe4845c.dirty


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