Hi,
this is the second version of this round of memory leak fixes.
There are only some smallish changes compared to v1:
- Explain leak checking a bit more carefully and document the new
`GIT_TEST_PASSING_SANITIZE_LEAK=check-failing` value in t/README. - Some more trivial commit message improvements.
Thanks!
Patrick
Patrick Steinhardt (22):
t/test-lib: allow skipping leak checks for passing tests
fetch-pack: fix memory leaks on fetch negotiation
send-pack: fix leaking common object IDs
builtin/push: fix leaking refspec query result
upload-pack: fix leaking child process data on reachability checks
submodule: fix leaking fetch task data
builtin/submodule--helper: fix leaking refs on push-check
remote: fix leaking tracking refs
remote: fix leak in reachability check of a remote-tracking ref
send-pack: fix leaking push cert nonce
gpg-interface: fix misdesigned signing key interfaces
object: clear grafts when clearing parsed object pool
shallow: free grafts when unregistering them
shallow: fix leaking members of `struct shallow_info`
negotiator/skipping: fix leaking commit entries
builtin/repack: fix leaking line buffer when packing promisors
builtin/pack-objects: plug leaking list of keep-packs
builtin/grep: fix leaking object context
builtin/fmt-merge-msg: fix leaking buffers
match-trees: fix leaking prefixes in `shift_tree()`
merge-ort: fix two leaks when handling directory rename modifications
builtin/repack: fix leaking keep-pack list
builtin/fmt-merge-msg.c | 2 ++
builtin/grep.c | 1 +
builtin/pack-objects.c | 1 +
builtin/push.c | 8 +++--
builtin/repack.c | 3 ++
builtin/submodule--helper.c | 2 ++
builtin/tag.c | 3 +-
commit.c | 23 ++++--------
commit.h | 3 +-
fetch-pack.c | 3 ++
gpg-interface.c | 26 ++++++++------
gpg-interface.h | 4 +--
match-trees.c | 10 ++++--
merge-ort.c | 4 ++-
negotiator/skipping.c | 7 ++--
object.c | 14 +++++++-
object.h | 4 ++-
remote.c | 6 +++-
repository.c | 2 +-
send-pack.c | 52 ++++++++++++++++++----------
shallow.c | 15 ++++++--
submodule.c | 2 ++
t/README | 3 ++
t/t5516-fetch-push.sh | 1 +
t/t5526-fetch-submodules.sh | 1 +
t/t5531-deep-submodule-push.sh | 1 +
t/t5533-push-cas.sh | 1 +
t/t5534-push-signed.sh | 1 +
t/t5537-fetch-shallow.sh | 1 +
t/t5538-push-shallow.sh | 1 +
t/t5549-fetch-push-http.sh | 1 +
t/t5552-skipping-fetch-negotiator.sh | 2 ++
t/t5616-partial-clone.sh | 1 +
t/t6132-pathspec-exclude.sh | 1 +
t/t6135-pathspec-with-attrs.sh | 2 ++
t/t6200-fmt-merge-msg.sh | 1 +
t/t6409-merge-subtree.sh | 1 +
t/t6423-merge-rename-directories.sh | 1 +
t/t6500-gc.sh | 1 +
t/t7703-repack-geometric.sh | 1 +
t/test-lib.sh | 11 +++++-
upload-pack.c | 22 ++++++++----
42 files changed, 178 insertions(+), 72 deletions(-)
Range-diff against v1:
1: 7c158acadf4 ! 1: fa62d0106a5 t/test-lib: allow skipping leak checks for passing tests
@@ Commit message
With `GIT_TEST_PASSING_SANITIZE_LEAK=check`, one can double check
whether a memory leak fix caused some test suites to become leak free.
+ This is done by running all tests with the leak checker enabled. If a
+ test suite does not declare `TEST_PASSES_SANITIZE_LEAK=true` but still
+ finishes successfully with the leak checker enabled, then this indicates
+ that the test is leak free and thus missing the annotation.
+
It is somewhat slow to execute though because it runs all of our test
suites with the leak sanitizer enabled. It is also pointless in most
cases, because the only test suites that need to be checked are those
which _aren't_ yet marked with `TEST_PASSES_SANITIZE_LEAK=true`.
- Introduce a new value "check-failing". If set, we will only check those
- tests which are not yet marked as leak free.
+ Introduce a new value "check-failing". When set, we behave the same as
+ if "check" was passed, except that we only check those tests which do
+ not have `TEST_PASSES_SANITIZE_LEAK=true` set. This is significantly
+ faster than running all test suites but still fulfills the usecase of
+ finding newly-leak-free test suites.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
+ ## t/README ##
+@@ t/README: GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
+ will run to completion faster, and result in the same failing
+ tests.
+
++GIT_TEST_PASSING_SANITIZE_LEAK=check-failing behaves the same as "check",
++but skips all tests which are already marked as leak-free.
++
+ GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
+ default to n.
+
+
## t/test-lib.sh ##
@@ t/test-lib.sh: then
passes_sanitize_leak=t
2: 33bc728a345 = 2: 611a29d1ca3 fetch-pack: fix memory leaks on fetch negotiation
3: a13db9777f0 = 3: 0d969962a39 send-pack: fix leaking common object IDs
4: 92fc97b3db8 = 4: 834a184c855 builtin/push: fix leaking refspec query result
5: 77023421e18 = 5: 17b0c4b577a upload-pack: fix leaking child process data on reachability checks
6: b4b65c3783c = 6: 872f39faece submodule: fix leaking fetch task data
7: e3d1ac1712f = 7: 3cbd6fe808e builtin/submodule--helper: fix leaking refs on push-check
8: 7fafcc53d23 = 8: 90647301de5 remote: fix leaking tracking refs
9: 1446e42f0ba = 9: bb845fc9ff1 remote: fix leak in reachability check of a remote-tracking ref
10: 138a5ded35a ! 10: a1baba39bc5 send-pack: fix leaking push cert nonce
@@ Commit message
When retrieving the push cert nonce from the server, we first store the
constant returned by `server_feature_value()` and then, if the nonce is
- valid, we duplicate the nonce memory to extend its lifetime. We never
- free the latter and thus cause a memory leak.
+ valid, we duplicate the nonce memory to a NUL-terminated string, so that
+ we can pass it to `generate_push_cert()`. We never free the latter and
+ thus cause a memory leak.
Fix this by storing the limited-lifetime nonce into a scope-local
variable such that the long-lived, allocated nonce can be easily freed
11: a1fca8104b3 = 11: ddebe2f6f6b gpg-interface: fix misdesigned signing key interfaces
12: 922b8640a55 = 12: 242f6b76db3 object: clear grafts when clearing parsed object pool
13: ec9a5143241 = 13: 4747042cb6a shallow: free grafts when unregistering them
14: 2a63030ff09 = 14: d3996c92d80 shallow: fix leaking members of `struct shallow_info`
15: 920db3a2912 = 15: 66ed1151449 negotiator/skipping: fix leaking commit entries
16: 19eb9073482 = 16: 78c1e5e1772 builtin/repack: fix leaking line buffer when packing promisors
17: 017713f5a9c = 17: d2e108040fd builtin/pack-objects: plug leaking list of keep-packs
18: 0722cb38ea9 ! 18: 9fd891f5222 builtin/grep: fix leaking object context
@@ Commit message
builtin/grep: fix leaking object context
Even when `get_oid_with_context()` fails it may have allocated some data
- in tthe object context. But we do not release it in git-grep(1) when the
+ in the object context. But we do not release it in git-grep(1) when the
call fails, leading to a memory leak. Plug it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
19: b37391c0d6b = 19: 84b68affa0d builtin/fmt-merge-msg: fix leaking buffers
20: 05461e3b1c0 = 20: bbb8ab20229 match-trees: fix leaking prefixes in `shift_tree()`
21: da1c23a9ccf = 21: db0e7a8733a merge-ort: fix two leaks when handling directory rename modifications
22: 3d30c727fbc = 22: 2368924995f builtin/repack: fix leaking keep-pack listbase-commit: 098ca6ba562006675df6a6d0948400d2d955458b
Patrick Steinhardt <ps@pks.im>