mbox series

[v2,00/22] Memory leak fixes (pt.6)

Message ID cover.1725530720.git.ps@pks.im (mailing list archive)
Headers show
Series Memory leak fixes (pt.6) | expand

Message

Patrick Steinhardt Sept. 5, 2024, 10:08 a.m. UTC 
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 list

base-commit: 098ca6ba562006675df6a6d0948400d2d955458b

Comments

Junio C Hamano Sept. 8, 2024, 9:39 p.m. UTC | #1 
Patrick Steinhardt <ps@pks.im> writes:

> 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.

These changes looked trivially good ;-)  Shall we mark the topic for
'next' soonish, or are there others who want to comment?

Thanks.
Junio C Hamano Sept. 12, 2024, 8:29 p.m. UTC | #2 
Patrick Steinhardt <ps@pks.im> writes:

> 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!

Looking good.  Let me mark the topic for 'next' soonish.

Thanks.