[PATCH v3 00/21] Various memory leak fixes
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 27, 2024, 11:45 UTC
- Message-ID
- <cover.1716810168.git.ps@pks.im>
- In-Reply-To
- <cover.1716465556.git.ps@pks.im>
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 pathsMakefile | 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