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