Memory leak fixes (pt.6)
cover Memory leak fixes (pt.6)
Hi,
here's another round of memory leak fixes. This is the second-last round of "general" leak fixes -- so once this and the next series are merged, I have made a full pass over all failing test suites. There will still be a bunch of memory leaks, namely ~86 failing test suites.
I have documented a bunch of remaining leaks at [1]. Most of them are a bit more involved to fix, so they will likely require separate series. These issues relate to:
- Simplifying commit history causes us to rewrite their parents, but we don't free the old parents in some cases. Hit e.g. by t6012.
- The describe atom infrastructure as used by git-for-each-ref(1) is
leaky. Hit e.g. by t6300, t6302.
- Breaking rewrites causes memory leaks. Hit e.g. by t7524.
- Cloning repositories causes us to re-initialize the_repository. This surfaces lifetime management issues with the struct remote that we use, which reuses storage held by the_repository. Hit e.g. by t5558.
- The GPG interface is leaky. Hit e.g. by t7510, t7528.
If you want to do some puzzling, please feel free to pick up any of these leaks :) I won't start working on them before I have the final part of memory leak fixes merged.
This patch series builds on top of 6a09c36371 (The eighth batch, 2024-08-23) with Junio's ps/leakfixes-part-5 at 13b23d2da5 (transport:
fix leaking negotiation tips, 2024-08-22) merged into it.
Patrick
[1]: https://gitlab.com/groups/gitlab-org/-/epics/14943
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/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 ++++++++---- 41 files changed, 175 insertions(+), 72 deletions(-)
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 1/22 t/test-lib: allow skipping leak checks for passing tests
With GIT_TEST_PASSING_SANITIZE_LEAK=check, one can double check whether a memory leak fix caused some test suites to become leak free. 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.
--- t/test-lib.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
t/test-lib.sh +10 -1
@@ -1558,8 +1558,16 @@ then passes_sanitize_leak=t fi - if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" then + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" && + test -n "$passes_sanitize_leak" + then + skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing" + test_done + fi + sanitize_leak_check=t if test -n "$invert_exit_code" then
@@ -1597,6 +1605,7 @@ then export LSAN_OPTIONS elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" || test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false then BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 2/22 fetch-pack: fix memory leaks on fetch negotiation
In reply to Patrick SteinhardtWe leak both the nt_object_array and negotiator structures in negotiate_using_fetch(). Plug both of these leaks.
These leaks were exposed by t5516, but fixing them is not sufficient to make the whole test suite leak free.
--- fetch-pack.c | 3 +++ 1 file changed, 3 insertions(+)
fetch-pack.c +3 -0
@@ -2227,7 +2227,10 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,trace2_region_leave ("fetch-pack" ,"negotiate_using_fetch" , the_repository);trace2_data_intmax ("negotiate_using_fetch" , the_repository,"total_rounds" , negotiation_round); +clear_common_flag (acked_commits); +object_array_clear (&nt_object_array); + negotiator.release (&negotiator);strbuf_release (&req_buf); }
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 3/22 send-pack: fix leaking common object IDs
In reply to Patrick SteinhardtWe're leaking the array of common object IDs in send_pack(). Fix this by creating a common exit path where we free the leaking data. While at it, unify some other cleanups now that we have a central place to put them.
--- send-pack.c | 34 ++++++++++++++++++++++------------ t/t5549-fetch-push-http.sh | 1 + 2 files changed, 23 insertions(+), 12 deletions(-)
send-pack.c +22 -12
@@ -508,7 +508,8 @@ int send_pack(struct send_pack_args *args,if (!remote_refs) {fprintf (stderr,"No refs in common and none specified; doing nothing.\n" "Perhaps you should specify a branch.\n" ); -return 0 ; + ret =0 ; + goto out; }git_config_get_bool ("push.negotiate" , &push_negotiate);
@@ -615,12 +616,11 @@ int send_pack(struct send_pack_args *args,* atomically, abort the whole operation. */ if (use_atomic) { -strbuf_release (&req_buf); -strbuf_release (&cap_buf);reject_atomic_push (remote_refs, args->send_mirror);error ("atomic push failed for ref %s. status: %d\n" , ref->name, ref->status); -return args->porcelain ?0 :-1 ; + ret = args->porcelain ?0 :-1 ; + goto out; }/* else fallthrough */ default :
@@ -682,8 +682,6 @@ int send_pack(struct send_pack_args *args,write_or_die (out, req_buf.buf, req_buf.len);packet_flush (out); } -strbuf_release (&req_buf); -strbuf_release (&cap_buf);if (use_sideband && cmds_sent) {memset (&demux,0 ,sizeof (demux));
@@ -721,7 +719,9 @@ int send_pack(struct send_pack_args *args,finish_async (&demux); } fd[1 ] =-1 ; -return -1 ; + + ret =-1 ; + goto out; }if (!args->stateless_rpc)/* Closed by pack_objects() via start_command() */
@@ -746,10 +746,12 @@ int send_pack(struct send_pack_args *args, }if (ret <0 ) -return ret; + goto out; -if (args->porcelain) -return 0 ; +if (args->porcelain) { + ret =0 ; + goto out; + }for (ref = remote_refs; ref; ref = ref->next) {switch (ref->status) {
@@ -758,8 +760,16 @@ int send_pack(struct send_pack_args *args,case REF_STATUS_OK :break ;default : -return -1 ; + ret =-1 ; + goto out; } } -return 0 ; + + ret =0 ; + +out: +oid_array_clear (&commons); +strbuf_release (&req_buf); +strbuf_release (&cap_buf); +return ret; }
t/t5549-fetch-push-http.sh +1 -0
@@ -5,6 +5,7 @@ test_description='fetch/push functionality using the HTTP protocol' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 4/22 builtin/push: fix leaking refspec query result
In reply to Patrick SteinhardtWhen appending a refspec via refspec_append_mapped() we leak the result of query_refspecs(). The overall logic around refspec queries is quite weird, as callers are expected to either set the src or dst pointers, and then the (allocated) result will be in the respective other struct member.
As we have the src member set, plugging the memory leak is thus as easy as just freeing the dst member. While at it, use designated initializers to initialize the structure.
This leak was exposed by t5516, but fixing it is not sufficient to make the whole test suite leak free.
--- builtin/push.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
builtin/push.c +5 -3
@@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,const char *branch_name;if (remote->push.nr) { -struct refspec_item query; -memset (&query,0 ,sizeof (struct refspec_item )); - query.src = matched->name; +struct refspec_item query = { + .src = matched->name, + }; +if (!query_refspecs (&remote->push, &query) && query.dst) {refspec_appendf (refspec,"%s%s:%s" , query.force ?"+" :"" , query.src, query.dst); +free (query.dst);return ; } }
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 5/22 upload-pack: fix leaking child process data on reachability checks
In reply to Patrick SteinhardtWe spawn a git-rev-list(1) command to perform reachability checks in "upload-pack.c". We do not release memory associated with the process in error cases though, thus leaking memory.
Fix these by calling child_process_clear().
--- t/t5516-fetch-push.sh | 1 + upload-pack.c | 22 ++++++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-)
t/t5516-fetch-push.sh +1 -0
@@ -19,6 +19,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh D=$(pwd)
upload-pack.c +16 -6
@@ -709,10 +709,13 @@ static int get_reachable_list(struct upload_pack_data *data,struct object *o;char namebuf[GIT_MAX_HEXSZ +2 ];/* ^ + hash + LF */ const unsigned hexsz = the_hash_algo->hexsz; +int ret;if (do_reachable_revlist (&cmd, &data->shallows, reachable, - data->allow_uor) <0 ) -return -1 ; + data->allow_uor) <0 ) { + ret =-1 ; + goto out; + }while ((i =read_in_full (cmd.out, namebuf, hexsz +1 )) == hexsz +1 ) {struct object_id oid;
@@ -736,10 +739,16 @@ static int get_reachable_list(struct upload_pack_data *data, }close (cmd.out); -if (finish_command (&cmd)) -return -1 ; +if (finish_command (&cmd)) { + ret =-1 ; + goto out; + } -return 0 ; + ret =0 ; + +out: +child_process_clear (&cmd); +return ret; }static int has_unreachable (struct object_array *src,enum allow_uor allow_uor)
@@ -749,7 +758,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)int i;if (do_reachable_revlist (&cmd, src,NULL , allow_uor) <0 ) -return 1 ; + goto error;/* * The commits out of the rev-list are not ancestors of
@@ -775,6 +784,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor) error:if (cmd.out >=0 )close (cmd.out); +child_process_clear (&cmd);return 1 ; }
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 6/22 submodule: fix leaking fetch task data
In reply to Patrick SteinhardtThe submodule_parallel_fetch structure contains various data structures that we use to set up parallel fetches of submodules. We do not free some of its data though, causing memory leaks. Plug those.
--- submodule.c | 2 ++ t/t5526-fetch-submodules.sh | 1 + 2 files changed, 3 insertions(+)
submodule.c +2 -0
@@ -1883,6 +1883,8 @@ int fetch_submodules(struct repository *r, out:free_submodules_data (&spf.changed_submodule_names);string_list_clear (&spf.seen_submodule_names,0 ); +strbuf_release (&spf.submodules_with_errors); +free (spf.oid_fetch_tasks);return spf.result; }
t/t5526-fetch-submodules.sh +1 -0
@@ -6,6 +6,7 @@ test_description='Recursive "git fetch" for submodules' GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh pwd=$(pwd)
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 7/22 builtin/submodule--helper: fix leaking refs on push-check
In reply to Patrick SteinhardtIn the push-check subcommand of the submodule helper we acquire a list of local refs, but never free that list. Fix this memory leak.
--- builtin/submodule--helper.c | 2 ++ t/t5531-deep-submodule-push.sh | 1 + 2 files changed, 3 insertions(+)
builtin/submodule--helper.c +2 -0
@@ -2958,7 +2958,9 @@ static int push_check(int argc, const char **argv, const char *prefix UNUSED) rs->src); } } +refspec_clear (&refspec); +free_refs (local_refs); }free (head);
t/t5531-deep-submodule-push.sh +1 -0
@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup '
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 8/22 remote: fix leaking tracking refs
In reply to Patrick SteinhardtWhen computing the remote tracking ref we cause two memory leaks:
- We leak when remote_tracking() fails.
- We leak when the call to remote_tracking() succeeds and sets
`ref->tracking_ref()`.
Fix both of these leaks.
--- remote.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
remote.c +4 -1
@@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)return ;free_one_ref (ref->peer_ref);free (ref->remote_status); +free (ref->tracking_ref);free (ref->symref);free (ref); }
@@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname, dst =apply_refspecs (&remote->fetch, refname);if (!dst)return -1 ;/* no tracking ref for refname at remote */ -if (refs_read_ref (get_main_ref_store (the_repository), dst, oid)) +if (refs_read_ref (get_main_ref_store (the_repository), dst, oid)) { +free (dst);return -1 ;/* we know what the tracking ref is but we cannot read it */ + } *dst_refname = dst;return 0 ;
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 9/22 remote: fix leak in reachability check of a remote-tracking ref
In reply to Patrick SteinhardtIn check_if_includes_upstream() we retrieve the local ref corresponding to a remote-tracking ref we want to check reachability for. We never free that local ref and thus cause a memory leak. Fix this.
--- remote.c | 1 + t/t5533-push-cas.sh | 1 + 2 files changed, 2 insertions(+)
remote.c +1 -0
@@ -2774,6 +2774,7 @@ static void check_if_includes_upstream(struct ref *remote)if (is_reachable_in_reflog (local->name, remote) <=0 ) remote->unreachable =1 ; +free_one_ref (local); } static void apply_cas(struct push_cas_option *cas,
t/t5533-push-cas.sh +1 -0
@@ -5,6 +5,7 @@ test_description='compare & swap push force/delete safety' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh setup_srcdst_basic () {
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 10/22 send-pack: fix leaking push cert nonce
In reply to Patrick SteinhardtWhen 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.
Fix this by storing the limited-lifetime nonce into a scope-local variable such that the long-lived, allocated nonce can be easily freed without having to cast away its constness.
This leak was exposed by t5534, but fixing it is not sufficient to make the whole test suite leak free.
--- send-pack.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
send-pack.c +7 -5
@@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,unsigned cmds_sent =0 ;int ret;struct async demux; -const char *push_cert_nonce =NULL ; +char *push_cert_nonce =NULL ;struct packet_reader reader;int use_bitmaps;
@@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,if (args->push_cert !=SEND_PACK_PUSH_CERT_NEVER ) {size_t len; - push_cert_nonce =server_feature_value ("push-cert" , &len); -if (push_cert_nonce) { -reject_invalid_nonce (push_cert_nonce, len); - push_cert_nonce =xmemdupz (push_cert_nonce, len); +const char *nonce =server_feature_value ("push-cert" , &len); + +if (nonce) { +reject_invalid_nonce (nonce, len); + push_cert_nonce =xmemdupz (nonce, len); }else if (args->push_cert ==SEND_PACK_PUSH_CERT_ALWAYS ) {die (_ ("the receiving end does not support --signed push" )); }else if (args->push_cert ==SEND_PACK_PUSH_CERT_IF_ASKED ) {
@@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,oid_array_clear (&commons);strbuf_release (&req_buf);strbuf_release (&cap_buf); +free (push_cert_nonce);return ret; }
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 11/22 gpg-interface: fix misdesigned signing key interfaces
In reply to Patrick SteinhardtThe interfaces to retrieve signing keys and their IDs are misdesigned as they return string constants even though they indeed allocate memory, which leads to memory leaks. Refactor the code to instead always return allocated strings and let the callers free them accordingly.
--- builtin/tag.c | 3 ++- commit.c | 9 ++++++--- gpg-interface.c | 26 +++++++++++++++----------- gpg-interface.h | 4 ++-- send-pack.c | 6 ++++-- t/t5534-push-signed.sh | 1 + 6 files changed, 30 insertions(+), 19 deletions(-)
builtin/tag.c +2 -1
@@ -160,7 +160,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,const struct git_hash_algo *compat = the_repository->compat_hash_algo;struct strbuf sig =STRBUF_INIT , compat_sig =STRBUF_INIT ;struct strbuf compat_buf =STRBUF_INIT ; -const char *keyid =get_signing_key (); +char *keyid =get_signing_key ();int ret =-1 ;if (sign_buffer (buffer, &sig, keyid))
@@ -190,6 +190,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,strbuf_release (&sig);strbuf_release (&compat_sig);strbuf_release (&compat_buf); +free (keyid);return ret; }
commit.c +6 -3
@@ -1150,11 +1150,14 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gistatic int sign_commit_to_strbuf (struct strbuf *sig,struct strbuf *buf,const char *keyid) { +char *keyid_to_free =NULL ; +int ret =0 ;if (!keyid || !*keyid) - keyid =get_signing_key (); + keyid = keyid_to_free =get_signing_key ();if (sign_buffer (buf, sig, keyid)) -return -1 ; -return 0 ; + ret =-1 ; +free (keyid_to_free); +return ret; } int parse_signed_commit(const struct commit *commit,
gpg-interface.c +15 -11
@@ -45,8 +45,8 @@ struct gpg_format { size_t signature_size);int (*sign_buffer)(struct strbuf *buffer,struct strbuf *signature,const char *signing_key); -const char *(*get_default_key)(void ); -const char *(*get_key_id)(void ); +char *(*get_default_key)(void ); +char *(*get_key_id)(void ); };static const char *openpgp_verify_args[] = {
@@ -86,9 +86,9 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,static int sign_buffer_ssh (struct strbuf *buffer,struct strbuf *signature,const char *signing_key); -static const char *get_default_ssh_signing_key (void ); +static char *get_default_ssh_signing_key (void ); -static const char *get_ssh_key_id (void ); +static char *get_ssh_key_id (void );static struct gpg_format gpg_format[] = { {
@@ -847,7 +847,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key) }/* Returns the first public key from an ssh-agent to use for signing */ -static const char *get_default_ssh_signing_key (void ) +static char *get_default_ssh_signing_key (void ) {struct child_process ssh_default_key =CHILD_PROCESS_INIT ;int ret =-1 ;
@@ -899,12 +899,16 @@ static const char *get_default_ssh_signing_key(void)return default_key; } -static const char *get_ssh_key_id (void ) { -return get_ssh_key_fingerprint (get_signing_key ()); +static char *get_ssh_key_id (void ) +{ +char *signing_key =get_signing_key (); +char *key_id =get_ssh_key_fingerprint (signing_key); +free (signing_key); +return key_id; }/* Returns a textual but unique representation of the signing key */ -const char *get_signing_key_id (void ) +char *get_signing_key_id (void ) {gpg_interface_lazy_init ();
@@ -916,17 +920,17 @@ const char *get_signing_key_id(void)return get_signing_key (); } -const char *get_signing_key (void ) +char *get_signing_key (void ) {gpg_interface_lazy_init ();if (configured_signing_key) -return configured_signing_key; +return xstrdup (configured_signing_key);if (use_format->get_default_key) {return use_format->get_default_key (); } -return git_committer_info (IDENT_STRICT |IDENT_NO_DATE ); +return xstrdup (git_committer_info (IDENT_STRICT |IDENT_NO_DATE )); }const char *gpg_trust_level_to_str (enum signature_trust_level level)
gpg-interface.h +2 -2
@@ -80,13 +80,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,const char *gpg_trust_level_to_str (enum signature_trust_level level);void set_signing_key (const char *); -const char *get_signing_key (void ); +char *get_signing_key (void );/* * Returns a textual unique representation of the signing key in use * Either a GPG KeyID or a SSH Key Fingerprint */ -const char *get_signing_key_id (void ); +char *get_signing_key_id (void );int check_signature (struct signature_check *sigc,const char *signature,size_t slen); void print_signature_buffer(const struct signature_check *sigc,
send-pack.c +4 -2
@@ -348,7 +348,8 @@ static int generate_push_cert(struct strbuf *req_buf, {const struct ref *ref;struct string_list_item *item; -char *signing_key_id =xstrdup (get_signing_key_id ()); +char *signing_key_id =get_signing_key_id (); +char *signing_key =get_signing_key ();const char *cp, *np;struct strbuf cert =STRBUF_INIT ;int update_seen =0 ;
@@ -381,7 +382,7 @@ static int generate_push_cert(struct strbuf *req_buf,if (!update_seen) goto free_return; -if (sign_buffer (&cert, &cert,get_signing_key ())) +if (sign_buffer (&cert, &cert, signing_key))die (_ ("failed to sign the push certificate" ));packet_buf_write (req_buf,"push-cert%c%s" ,0 , cap_string);
@@ -394,6 +395,7 @@ static int generate_push_cert(struct strbuf *req_buf, free_return:free (signing_key_id); +free (signing_key);strbuf_release (&cert);return update_seen; }
t/t5534-push-signed.sh +1 -0
@@ -5,6 +5,7 @@ test_description='signed push' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-gpg.sh
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 12/22 object: clear grafts when clearing parsed object pool
In reply to Patrick SteinhardtWe do not clear grafts part of the parsed object pool when clearing the pool itself, which can lead to memory leaks when a repository is being cleared.
Fix this by moving reset_commit_grafts() into "object.c" and making it part of the struct parsed_object_pool interface such that we can call it from parsed_object_pool_clear(). Adapt parsed_object_pool_new() to take and store a reference to its owning repository, which is needed by unparse_commit().
--- commit.c | 14 +------------- commit.h | 3 ++- object.c | 14 +++++++++++++- object.h | 4 +++- repository.c | 2 +- shallow.c | 2 +- 6 files changed, 21 insertions(+), 18 deletions(-)
commit.c +1 -13
@@ -177,7 +177,7 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid) commit_graft_oid_access); } -static void unparse_commit (struct repository *r,const struct object_id *oid) +void unparse_commit (struct repository *r,const struct object_id *oid) {struct commit *c =lookup_commit (r, oid);
@@ -318,18 +318,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)return ret; } -void reset_commit_grafts (struct repository *r) -{ -int i; - -for (i =0 ; i < r->parsed_objects->grafts_nr; i++) { -unparse_commit (r, &r->parsed_objects->grafts[i]->oid); -free (r->parsed_objects->grafts[i]); - } - r->parsed_objects->grafts_nr =0 ; - r->parsed_objects->commit_graft_prepared =0 ; -} -struct commit_buffer {void *buffer;unsigned long size;
commit.h +2 -1
@@ -108,6 +108,8 @@ static inline int repo_parse_commit_no_graph(struct repository *r,void parse_commit_or_die (struct commit *item); +void unparse_commit (struct repository *r,const struct object_id *oid); +struct buffer_slab ;struct buffer_slab *allocate_commit_buffer_slab (void );void free_commit_buffer_slab (struct buffer_slab *bs);
@@ -240,7 +242,6 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid);int register_commit_graft (struct repository *r,struct commit_graft *,int );void prepare_commit_graft (struct repository *r);struct commit_graft *lookup_commit_graft (struct repository *r,const struct object_id *oid); -void reset_commit_grafts (struct repository *r);struct commit *get_fork_point (const char *refname,struct commit *commit);
object.c +13 -1
@@ -545,11 +545,12 @@ void repo_clear_commit_marks(struct repository *r, unsigned int flags) } } -struct parsed_object_pool *parsed_object_pool_new (void ) +struct parsed_object_pool *parsed_object_pool_new (struct repository *repo) {struct parsed_object_pool *o =xmalloc (sizeof (*o));memset (o,0 ,sizeof (*o)); + o->repo = repo; o->blob_state =allocate_alloc_state (); o->tree_state =allocate_alloc_state (); o->commit_state =allocate_alloc_state ();
@@ -628,6 +629,16 @@ void raw_object_store_clear(struct raw_object_store *o)hashmap_clear (&o->pack_map); } +void parsed_object_pool_reset_commit_grafts (struct parsed_object_pool *o) +{ +for (int i =0 ; i < o->grafts_nr; i++) { +unparse_commit (o->repo, &o->grafts[i]->oid); +free (o->grafts[i]); + } + o->grafts_nr =0 ; + o->commit_graft_prepared =0 ; +} +void parsed_object_pool_clear (struct parsed_object_pool *o) {/*
@@ -659,6 +670,7 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)free_commit_buffer_slab (o->buffer_slab); o->buffer_slab =NULL ; +parsed_object_pool_reset_commit_grafts (o);clear_alloc_state (o->blob_state);clear_alloc_state (o->tree_state);clear_alloc_state (o->commit_state);
object.h +3 -1
@@ -7,6 +7,7 @@ struct buffer_slab;struct repository ;struct parsed_object_pool { +struct repository *repo;struct object **obj_hash;int nr_objs, obj_hash_size;
@@ -31,8 +32,9 @@ struct parsed_object_pool {struct buffer_slab *buffer_slab; }; -struct parsed_object_pool *parsed_object_pool_new (void ); +struct parsed_object_pool *parsed_object_pool_new (struct repository *repo);void parsed_object_pool_clear (struct parsed_object_pool *o); +void parsed_object_pool_reset_commit_grafts (struct parsed_object_pool *o);struct object_list {struct object *item;
repository.c +1 -1
@@ -54,7 +54,7 @@ void initialize_repository(struct repository *repo) { repo->objects =raw_object_store_new (); repo->remote_state =remote_state_new (); - repo->parsed_objects =parsed_object_pool_new (); + repo->parsed_objects =parsed_object_pool_new (repo);ALLOC_ARRAY (repo->index,1 );index_state_init (repo->index, repo);
shallow.c +1 -1
@@ -97,7 +97,7 @@ static void reset_repository_shallow(struct repository *r) { r->parsed_objects->is_shallow =-1 ;stat_validity_clear (r->parsed_objects->shallow_stat); -reset_commit_grafts (r); +parsed_object_pool_reset_commit_grafts (r->parsed_objects); }int commit_shallow_file (struct repository *r,struct shallow_lock *lk)
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 13/22 shallow: free grafts when unregistering them
In reply to Patrick SteinhardtWhen removing a graft via unregister_shallow() we remove it from the grafts array, but do not free the structure. Fix this to plug the leak.
--- shallow.c | 4 +++- t/t5537-fetch-shallow.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
shallow.c +3 -1
@@ -51,10 +51,12 @@ int unregister_shallow(const struct object_id *oid)int pos =commit_graft_pos (the_repository, oid);if (pos <0 )return -1 ; -if (pos +1 < the_repository->parsed_objects->grafts_nr) +if (pos +1 < the_repository->parsed_objects->grafts_nr) { +free (the_repository->parsed_objects->grafts[pos]);MOVE_ARRAY (the_repository->parsed_objects->grafts + pos, the_repository->parsed_objects->grafts + pos +1 , the_repository->parsed_objects->grafts_nr - pos -1 ); + } the_repository->parsed_objects->grafts_nr--;return 0 ; }
t/t5537-fetch-shallow.sh +1 -0
@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh commit() {
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 14/22 shallow: fix leaking members of `struct shallow_info`
In reply to Patrick SteinhardtWe do not free several struct members in clear_shallow_info(). Fix this to plug the resulting leaks.
--- shallow.c | 9 +++++++++ t/t5538-push-shallow.sh | 1 + 2 files changed, 10 insertions(+)
shallow.c +9 -0
@@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)void clear_shallow_info (struct shallow_info *info) { +if (info->used_shallow) { +for (size_t i =0 ; i < info->shallow->nr; i++) +free (info->used_shallow[i]); +free (info->used_shallow); + } + +free (info->need_reachability_test); +free (info->reachable); +free (info->shallow_ref);free (info->ours);free (info->theirs); }
t/t5538-push-shallow.sh +1 -0
@@ -5,6 +5,7 @@ test_description='push from/to a shallow clone' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh commit() {
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 15/22 negotiator/skipping: fix leaking commit entries
In reply to Patrick SteinhardtWhen releasing the skipping negotiator we free its priority queue, but not the contained entries. Fix this to plug a memory leak.
--- negotiator/skipping.c | 7 +++++-- t/t5552-skipping-fetch-negotiator.sh | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-)
negotiator/skipping.c +5 -2
@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)static void release (struct fetch_negotiator *n) { -clear_prio_queue (&((struct data *)n->data)->rev_list); -FREE_AND_NULL (n->data); +struct data *data = n->data; +for (int i =0 ; i < data->rev_list.nr; i++) +free (data->rev_list.array[i].data); +clear_prio_queue (&data->rev_list); +FREE_AND_NULL (data); }void skipping_negotiator_init (struct fetch_negotiator *negotiator)
t/t5552-skipping-fetch-negotiator.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='test skipping fetch negotiator' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'fetch.negotiationalgorithm config' '
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 16/22 builtin/repack: fix leaking line buffer when packing promisors
In reply to Patrick SteinhardtIn repack_promisor_objects() we read output from git-pack-objects(1) line by line, using strbuf_getline_lf(). We never free the line buffer, causing a memory leak. Plug it.
This leak is being hit in t5616, but plugging it alone is not sufficient to make the whole test suite leak free.
--- builtin/repack.c | 2 ++ 1 file changed, 2 insertions(+)
builtin/repack.c +2 -0
@@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,free (promisor_name); } +fclose (out);if (finish_command (&cmd))die (_ ("could not finish pack-objects to repack promisor objects" )); +strbuf_release (&line); }struct pack_geometry {
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 17/22 builtin/pack-objects: plug leaking list of keep-packs
In reply to Patrick SteinhardtThe --keep-pack option of git-pack-objects(1) populates the arguments into a string list. And while the list is marked as NODUP and thus won't duplicate the strings, the list entries themselves still need to be free'd. We don't though, causing a leak.
Plug it.
--- builtin/pack-objects.c | 1 + t/t5616-partial-clone.sh | 1 + 2 files changed, 2 insertions(+)
builtin/pack-objects.c +1 -0
@@ -4641,6 +4641,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) cleanup:clear_packing_data (&to_pack);list_objects_filter_release (&filter_options); +string_list_clear (&keep_pack_list,0 );strvec_clear (&rp);return 0 ;
t/t5616-partial-clone.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git partial clone' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # create a normal "src" repo where we can later create new commits.
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 18/22 builtin/grep: fix leaking object context
In reply to Patrick SteinhardtEven 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 call fails, leading to a memory leak. Plug it.
--- builtin/grep.c | 1 + t/t6132-pathspec-exclude.sh | 1 + t/t6135-pathspec-with-attrs.sh | 2 ++ 3 files changed, 4 insertions(+)
builtin/grep.c +1 -0
@@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) &oid, &oc)) {if (seen_dashdash)die (_ ("unable to resolve revision: %s" ), arg); +object_context_release (&oc);break ; }
t/t6132-pathspec-exclude.sh +1 -0
@@ -2,6 +2,7 @@ test_description='test case exclude pathspec' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
t/t6135-pathspec-with-attrs.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='test labels in pathspecs' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup a tree' '
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 19/22 builtin/fmt-merge-msg: fix leaking buffers
In reply to Patrick SteinhardtFix leaking input and output buffers in git-fmt-merge-msg(1).
--- builtin/fmt-merge-msg.c | 2 ++ t/t6200-fmt-merge-msg.sh | 1 + 2 files changed, 3 insertions(+)
builtin/fmt-merge-msg.c +2 -0
@@ -67,6 +67,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)return ret;write_in_full (STDOUT_FILENO , output.buf, output.len); +strbuf_release (&input); +strbuf_release (&output);free (inpath);return 0 ; }
t/t6200-fmt-merge-msg.sh +1 -0
@@ -8,6 +8,7 @@ test_description='fmt-merge-msg test' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-gpg.sh"
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 20/22 match-trees: fix leaking prefixes in `shift_tree()`
In reply to Patrick SteinhardtIn shift_tree() we allocate two empty strings that we end up passing to match_trees(). If that function finds a better match it will update these pointers to point to a newly allocated strings, freeing the old strings. We never free the final results though, neither the ones we have allocated ourselves, nor the one that match_trees() might've returned to us.
Fix the resulting memory leaks by creating a common exit path where we free them.
--- match-trees.c | 10 +++++++--- t/t6409-merge-subtree.sh | 1 + 2 files changed, 8 insertions(+), 3 deletions(-)
match-trees.c +7 -3
@@ -294,18 +294,22 @@ void shift_tree(struct repository *r,unsigned short mode;if (!*del_prefix) -return ; + goto out;if (get_tree_entry (r, hash2, del_prefix, shifted, &mode))die ("cannot find path %s in tree %s" , del_prefix,oid_to_hex (hash2)); -return ; + goto out; }if (!*add_prefix) -return ; + goto out;splice_tree (hash1, add_prefix, hash2, shifted); + +out: +free (add_prefix); +free (del_prefix); }/*
t/t6409-merge-subtree.sh +1 -0
@@ -5,6 +5,7 @@ test_description='subtree merge strategy' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup '
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 21/22 merge-ort: fix two leaks when handling directory rename modifications
In reply to Patrick SteinhardtThere are two leaks in apply_directory_rename_modifications():
- We do not release the dirs_to_insert string list.
- We do not release some conflict_info we put into the
`opt->priv->paths` string map.
The former is trivial to fix. The latter is a bit less straight forward:
the util pointer of the string map may sometimes point to data that has been allocated via CALLOC(), while at other times it may point to data that has been allocated via a mem_pool.
It very much seems like an oversight that we didn't also allocate the conflict info in this code path via the memory pool, though. So let's fix that, which will also plug the memory leak for us.
--- merge-ort.c | 4 +++- t/t6423-merge-rename-directories.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
merge-ort.c +3 -1
@@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,struct conflict_info *dir_ci;char *cur_dir = dirs_to_insert.items[i].string; -CALLOC_ARRAY (dir_ci,1 ); + dir_ci =mem_pool_calloc (&opt->priv->pool,1 ,sizeof (*dir_ci)); dir_ci->merged.directory_name = parent_name; len =strlen (parent_name);
@@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,* Finally, record the new location. */ pair->two->path = new_path; + +string_list_clear (&dirs_to_insert,0 ); }/*** Function Grouping: functions related to regular rename detection ***/
t/t6423-merge-rename-directories.sh +1 -0
@@ -25,6 +25,7 @@ test_description="recursive merge with directory renames" # underscore notation is to differentiate different # files that might be renamed into each other's paths.) +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
PATCH 22/22 builtin/repack: fix leaking keep-pack list
In reply to Patrick SteinhardtThe list of packs to keep is populated via a command line option but never free'd. Plug this memory leak.
--- builtin/repack.c | 1 + t/t6500-gc.sh | 1 + t/t7703-repack-geometric.sh | 1 + 3 files changed, 3 insertions(+)
builtin/repack.c +1 -0
@@ -1543,6 +1543,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) } cleanup: +string_list_clear (&keep_pack_list,0 );string_list_clear (&names,1 );existing_packs_release (&existing);free_pack_geometry (&geometry);
t/t6500-gc.sh +1 -0
@@ -3,6 +3,7 @@ test_description='basic git gc tests ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh
t/t7703-repack-geometric.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git repack --geometric works correctly' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_TEST_MULTI_PACK_INDEX=0
Signature · 1 line
2.46.0.164.g477ce5ccd6.dirty
Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests
In reply to Patrick SteinhardtQuoted 9 lines · Patrick Steinhardt
With
GIT_TEST_PASSING_SANITIZE_LEAK=check, one can double check whether a memory leak fix caused some test suites to become leak free. 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 withTEST_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.
A very welcome addition. I am already liking it while running locally.
Thanks.
Quoted 36 lines
Signed-off-by Patrick Steinhardt <[email protected]>--- t/test-lib.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)t/test-lib.sh +10
-1@@ -1558,8 +1558,16 @@ then passes_sanitize_leak=t fi - if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" then + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" && + test -n "$passes_sanitize_leak" + then + skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing" + test_done + fi + sanitize_leak_check=t if test -n "$invert_exit_code" then@@ -1597,6 +1605,7 @@ then export LSAN_OPTIONS elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" || test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false then BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries
In reply to Patrick SteinhardtQuoted 16 lines · Patrick Steinhardt
When releasing the skipping negotiator we free its priority queue, but not the contained entries. Fix this to plug a memory leak.
Signed-off-by Patrick Steinhardt <[email protected]>--- negotiator/skipping.c | 7 +++++-- t/t5552-skipping-fetch-negotiator.sh | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-)negotiator/skipping.c +0
-0@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)static void release (struct fetch_negotiator *n)
negotiator/skipping.c static int ack(struct fetch_negotiator *n, struct commit *c) · line 256
Quoted 10 lines
negotiator/skipping.c +5
-2{ -clear_prio_queue (&((struct data *)n->data)->rev_list); -FREE_AND_NULL (n->data); +struct data *data = n->data; +for (int i =0 ; i < data->rev_list.nr; i++) +free (data->rev_list.array[i].data); +clear_prio_queue (&data->rev_list); +FREE_AND_NULL (data); }
It seems unintuitive that clear_prio_queue() doesn't also free the data underneath and that a caller would have to know to free that as well to avoid leaking memory. Would it make more sense to add this change to clear_prio_queue() instead? Patch 14 has that pattern already.
Thanks again for the cleanups -- I'm tempted to take a stab at some of the other memory leaks you mentioned during our biweekly hackathon. All of the other patches look reasonable to me.
Josh Steadmon
Reviewed-byRe: [PATCH 15/22] negotiator/skipping: fix leaking commit entries
Quoted 16 lines · Calvin Wan
Quoted 15 lines · Patrick Steinhardt
When releasing the skipping negotiator we free its priority queue, but not the contained entries. Fix this to plug a memory leak.
Signed-off-by Patrick Steinhardt <[email protected]>--- negotiator/skipping.c | 7 +++++-- t/t5552-skipping-fetch-negotiator.sh | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-)negotiator/skipping.c +0
-0@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
Quoted 16 lines
negotiator/skipping.c static int ack(struct fetch_negotiator *n, struct commit *c) · line 256
Quoted 11 lines
negotiator/skipping.c +5
-2static void release (struct fetch_negotiator *n) { -clear_prio_queue (&((struct data *)n->data)->rev_list); -FREE_AND_NULL (n->data); +struct data *data = n->data; +for (int i =0 ; i < data->rev_list.nr; i++) +free (data->rev_list.array[i].data); +clear_prio_queue (&data->rev_list); +FREE_AND_NULL (data); }It seems unintuitive that
clear_prio_queue()doesn't also free the data underneath and that a caller would have to know to free that as well to avoid leaking memory. Would it make more sense to add this change toclear_prio_queue()instead? Patch 14 has that pattern already.
I'm assuming the reasoning is that clear_prio_queue() can't know if its items need more complicated cleanup of their own, so if the caller (potentially) needs to clean up items individually anyway, the caller can also free them at the same time?
Quoted 3 lines
Thanks again for the cleanups -- I'm tempted to take a stab at some of the other memory leaks you mentioned during our biweekly hackathon. All of the other patches look reasonable to me.
The series also looks good to me, thanks!
Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries
Quoted 34 lines · Josh Steadmon
Quoted 16 lines · Calvin Wan
Quoted 15 lines · Patrick Steinhardt
When releasing the skipping negotiator we free its priority queue, but not the contained entries. Fix this to plug a memory leak.
Signed-off-by Patrick Steinhardt <[email protected]>--- negotiator/skipping.c | 7 +++++-- t/t5552-skipping-fetch-negotiator.sh | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-)negotiator/skipping.c +0
-0@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)Quoted 16 lines
negotiator/skipping.c static int ack(struct fetch_negotiator *n, struct commit *c) · line 256
Quoted 11 lines
negotiator/skipping.c +5
-2static void release (struct fetch_negotiator *n) { -clear_prio_queue (&((struct data *)n->data)->rev_list); -FREE_AND_NULL (n->data); +struct data *data = n->data; +for (int i =0 ; i < data->rev_list.nr; i++) +free (data->rev_list.array[i].data); +clear_prio_queue (&data->rev_list); +FREE_AND_NULL (data); }It seems unintuitive that
clear_prio_queue()doesn't also free the data underneath and that a caller would have to know to free that as well to avoid leaking memory. Would it make more sense to add this change toclear_prio_queue()instead? Patch 14 has that pattern already.
Quoted 4 lines
I'm assuming the reasoning is that
clear_prio_queue()can't know if its items need more complicated cleanup of their own, so if the caller (potentially) needs to clean up items individually anyway, the caller can also free them at the same time?
Yeah, that's mostly the reason. We have e.g. string_list_clear_func() that works around this issue by making the caller provide the cleanup function, and we could use the same pattern here. But it seems as if most of the callers of clear_prio_queue() don't need this because they already drain the queue during normal operations anyway.
With patch 14 you probably refer to clear_shallow_info()? We're not using a priority queue there, so it is not quite related to the prio queue we have here. So I'm inclined to leave this as-is, and if we ever see that we have more callsites that want to clean up the prio queue and its contents we can introduce prio_queue_clear_func(). Does that work for you?
Quoted 3 lines
Quoted 3 lines
Thanks again for the cleanups -- I'm tempted to take a stab at some of the other memory leaks you mentioned during our biweekly hackathon. All of the other patches look reasonable to me.
I'd certainly be happy to shed some of the load here. If you do, please give me a quick ping so that we don't duplicate any work by accident.
Quoted 3 lines
The series also looks good to me, thanks!
Reviewed-by Josh Steadmon <[email protected]>
Thanks!
Patrick
Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests
In reply to Patrick SteinhardtQuoted 6 lines · Patrick Steinhardt
With
GIT_TEST_PASSING_SANITIZE_LEAK=check, one can double check whether a memory leak fix caused some test suites to become leak free. 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 withTEST_PASSES_SANITIZE_LEAK=true.
What I understand from t/README the "check" value is used to test whether the presence or absence of TEST_PASSES_SANITIZE_LEAK=true matches the expectations. I think it's better to express that in the first sentence, because it sounds a bit misleading at the moment if you don't know that.
Quoted 2 lines
Introduce a new value "check-failing". If set, we will only check those tests which are not yet marked as leak free.
I would also mention this still has the effect that tests which *are* leak-free but do not have TEST_PASSES_SANITIZE_LEAK=true fail due to the use of "--invert-exit-code".
Also, can you add a short paragraph about this value in "t/README"?
--
Toon
Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`
In reply to Patrick SteinhardtQuoted 19 lines · Patrick Steinhardt
We do not free several struct members in
clear_shallow_info(). Fix this to plug the resulting leaks.Signed-off-by Patrick Steinhardt <[email protected]>--- shallow.c | 9 +++++++++ t/t5538-push-shallow.sh | 1 + 2 files changed, 10 insertions(+)shallow.c +2
-0@@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)void clear_shallow_info (struct shallow_info *info) { +if (info->used_shallow) { +for (size_t i =0 ; i < info->shallow->nr; i++)
shallow.c void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa) · line 503
Quoted 10 lines
shallow.c +7
-0+free (info->used_shallow[i]); +free (info->used_shallow); + } + +free (info->need_reachability_test); +free (info->reachable); +free (info->shallow_ref);free (info->ours);free (info->theirs); }
Recently was agreed in the CodingGuidelines S_clear() functions do a S_release() + S_init(). I see we're not initializing the struct for future use (i.e. we don't reset the nr_* fields to 0). But we cannot really do an init, because that would be calling prepare_shallow_info(), which allocates new memory. So would it be worth to rename this function to release_shallow_info()?
--
Toon
Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`
Quoted 9 lines · Toon claes
Quoted 9 lines
void
clear_shallow_info(struct shallow_info *info){shallow.c +7
-0+if (info->used_shallow) { +for (size_t i =0 ; i < info->shallow->nr; i++) +free (info->used_shallow[i]); +free (info->used_shallow); + } + +free (info->need_reachability_test);
Quoted 8 lines
shallow.c void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa) · line 503
Quoted 5 lines
shallow.c +2
-0+free (info->reachable); +free (info->shallow_ref);free (info->ours);free (info->theirs); }
prepare_shallow_info(), which allocates new memory. So would it be worth to rename this function torelease_shallow_info()?
In the longer term in a separate "renaming everything" effort, yes. In the context of "plug many resource leaks" series, probably no.
Thanks.
Re: [PATCH 15/22] negotiator/skipping: fix leaking commit entries
In reply to Patrick SteinhardtQuoted 7 lines · Patrick Steinhardt
Yeah, that's mostly the reason. We have e.g.
string_list_clear_func()that works around this issue by making the caller provide the cleanup function, and we could use the same pattern here. But it seems as if most of the callers ofclear_prio_queue()don't need this because they already drain the queue during normal operations anyway.
Quoted 6 lines
With patch 14 you probably refer to
clear_shallow_info()? We're not using a priority queue there, so it is not quite related to the prio queue we have here. So I'm inclined to leave this as-is, and if we ever see that we have more callsites that want to clean up the prio queue and its contents we can introduceprio_queue_clear_func(). Does that work for you?
Yes it does! Thanks for the clarification
Re: [PATCH 01/22] t/test-lib: allow skipping leak checks for passing tests
In reply to Toon claesQuoted 15 lines · Toon claes
Quoted 6 lines · Patrick Steinhardt
With
GIT_TEST_PASSING_SANITIZE_LEAK=check, one can double check whether a memory leak fix caused some test suites to become leak free. 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 withTEST_PASSES_SANITIZE_LEAK=true.What I understand from
t/READMEthe "check" value is used to test whether the presence or absence ofTEST_PASSES_SANITIZE_LEAK=truematches the expectations. I think it's better to express that in the first sentence, because it sounds a bit misleading at the moment if you don't know that.
Quoted 6 lines
Quoted 2 lines
Introduce a new value "check-failing". If set, we will only check those tests which are not yet marked as leak free.
I would also mention this still has the effect that tests which *are* leak-free but do not have
TEST_PASSES_SANITIZE_LEAK=truefail due to the use of "--invert-exit-code".
I don't want to go too much into the implementation details here, but will note that we continue to behave the same as with "check", except that we skip already-leak-free tests.
Quoted 1 line
Also, can you add a short paragraph about this value in "t/README"?
Yup, will do.
Patrick
Re: [PATCH 14/22] shallow: fix leaking members of `struct shallow_info`
In reply to Junio C HamanoQuoted 20 lines · Junio C Hamano
Quoted 9 lines · Toon claes
Quoted 9 lines
void
clear_shallow_info(struct shallow_info *info){shallow.c +7
-0+if (info->used_shallow) { +for (size_t i =0 ; i < info->shallow->nr; i++) +free (info->used_shallow[i]); +free (info->used_shallow); + } + +free (info->need_reachability_test);Quoted 8 lines
shallow.c void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa) · line 503
Quoted 5 lines
shallow.c +2
-0+free (info->reachable); +free (info->shallow_ref);free (info->ours);free (info->theirs); }
prepare_shallow_info(), which allocates new memory. So would it be worth to rename this function torelease_shallow_info()?
Quoted 2 lines
In the longer term in a separate "renaming everything" effort, yes. In the context of "plug many resource leaks" series, probably no.
Yeah, agreed. And note that release_shallow_info() does not match our style guide, either: it should be shallow_info_release().
Patrick
Re: [PATCH 04/22] builtin/push: fix leaking refspec query result
In reply to Patrick SteinhardtQuoted 5 lines · Patrick Steinhardt
When appending a refspec via
refspec_append_mapped()we leak the result ofquery_refspecs(). The overall logic around refspec queries is quite weird, as callers are expected to either set thesrcordstpointers, and then the (allocated) result will be in the respective other struct member.
Hmph, is it necessary to say "quite weird" for the purpose of this change? The query interface is designed to be usable to query both ways and within that constraints, I find it designed very nicely (but I do not think that is necessary to be said for the purpose of this change, either)..
Quoted 3 lines
As we have the
srcmember set, plugging the memory leak is thus as easy as just freeing thedstmember. While at it, use designated initializers to initialize the structure.
In order to understand this paragraph, of course, it helps for a reader to understand that the query_refspecs() gives an answer by populating the side other than the query side, and the answers are what we want to release.
Quoted 13 lines
This leak was exposed by t5516, but fixing it is not sufficient to make the whole test suite leak free.
Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/push.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)builtin/push.c +0
-0
builtin/push.c static void refspec_append_mapped(struct refspec *refspec, const char *ref, · line 77
Quoted 9 lines
builtin/push.c +3
-3@@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,const char *branch_name;if (remote->push.nr) { -struct refspec_item query; -memset (&query,0 ,sizeof (struct refspec_item )); - query.src = matched->name; +struct refspec_item query = { + .src = matched->name, + };
This is "while at it" change that does not contribute to the leak or the leakfix; the resulting code is easier to read.
builtin/push.c static void refspec_append_mapped(struct refspec *refspec, const char *ref, · line 83
Quoted 5 lines
builtin/push.c +1
-0if (!query_refspecs (&remote->push, &query) && query.dst) {refspec_appendf (refspec,"%s%s:%s" , query.force ?"+" :"" , query.src, query.dst); +free (query.dst);
And this is the real fix, which looks good.
Thanks.
Quoted 3 lines
builtin/push.c +0
-0return ; } }
Re: [PATCH 05/22] upload-pack: fix leaking child process data on reachability checks
In reply to Patrick SteinhardtQuoted 3 lines · Patrick Steinhardt
We spawn a git-rev-list(1) command to perform reachability checks in "
upload-pack.c". We do not release memory associated with the process in error cases though, thus leaking memory.
Thanks for plugging this leak dating back in ages.
Both changes look sensible to me.
Re: [PATCH 04/22] builtin/push: fix leaking refspec query result
In reply to Junio C HamanoQuoted 8 lines · Junio C Hamano
Quoted 5 lines · Patrick Steinhardt
When appending a refspec via
refspec_append_mapped()we leak the result ofquery_refspecs(). The overall logic around refspec queries is quite weird, as callers are expected to either set thesrcordstpointers, and then the (allocated) result will be in the respective other struct member.
Quoted 5 lines
Hmph, is it necessary to say "quite weird" for the purpose of this change? The query interface is designed to be usable to query both ways and within that constraints, I find it designed very nicely (but I do not think that is necessary to be said for the purpose of this change, either)..
I don't quite agree that it's nicely designed -- I find it rather hard to use and reason about, and the fact that so many callsites get this interface wrong seems to indicate that there is at least some sort of truth to this assessment.
But I don't mind removing this subjective opinion from the commit message. I'll do that in case I end up rerolling this patch series.
Thanks!
Patrick
Re: [PATCH 08/22] remote: fix leaking tracking refs
In reply to Patrick SteinhardtQuoted 10 lines · Patrick Steinhardt
remote.c +1
-0@@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)return ;free_one_ref (ref->peer_ref);free (ref->remote_status); +free (ref->tracking_ref);free (ref->symref);free (ref); }@@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname, dst =apply_refspecs (&remote->fetch, refname);
remote.c static int remote_tracking(struct remote *remote, const char *refname, · line 2630
Quoted 10 lines
remote.c +3
-1if (!dst)return -1 ;/* no tracking ref for refname at remote */ -if (refs_read_ref (get_main_ref_store (the_repository), dst, oid)) +if (refs_read_ref (get_main_ref_store (the_repository), dst, oid)) { +free (dst);return -1 ;/* we know what the tracking ref is but we cannot read it */ + } *dst_refname = dst;return 0 ;
Looking good.
Re: [PATCH 10/22] send-pack: fix leaking push cert nonce
In reply to Patrick SteinhardtQuoted 4 lines · Patrick Steinhardt
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.
"to extend its lifetime" -> "to a NUL-terminated string, so that we can pass it to generate_push_cert()".
What is going on in this code path is this.
The other side may send a nonce in the server capability. We die if that nonce is bogus. Otherwise we make a xmemdupz() copy because we need to pass the nonce to generate_push_cert() that expects nonce to be a NUL terminated string (the original server capability is a long concatenation of capabilities and we learn the <ptr, len> for the nonce). The function cryptographically signs the ref update request we have, together with the nonce we got from the server, so that the other side can validate that it is signed by us, and the nonce serves as a protection against replay attacks.
send-pack.c int send_pack(struct send_pack_args *args, · line 506
Quoted 12 lines
send-pack.c +1
-1@@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,unsigned cmds_sent =0 ;int ret;struct async demux; -const char *push_cert_nonce =NULL ; +char *push_cert_nonce =NULL ;struct packet_reader reader;int use_bitmaps;
This is a change necessary to avoid having to cast the parameter to free().
send-pack.c int send_pack(struct send_pack_args *args, · line 557
Quoted 13 lines
send-pack.c +5
-4@@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,if (args->push_cert !=SEND_PACK_PUSH_CERT_NEVER ) {size_t len; - push_cert_nonce =server_feature_value ("push-cert" , &len); -if (push_cert_nonce) { -reject_invalid_nonce (push_cert_nonce, len); - push_cert_nonce =xmemdupz (push_cert_nonce, len); +const char *nonce =server_feature_value ("push-cert" , &len); + +if (nonce) { +reject_invalid_nonce (nonce, len); + push_cert_nonce =xmemdupz (nonce, len);
And this hunk become needed because push_cert_nonce cannot receive the return value from server_feature_value() without stripping constness.
send-pack.c int send_pack(struct send_pack_args *args, · line 775
Quoted 8 lines
send-pack.c +1
-0}else if (args->push_cert ==SEND_PACK_PUSH_CERT_ALWAYS ) {die (_ ("the receiving end does not support --signed push" )); }else if (args->push_cert ==SEND_PACK_PUSH_CERT_IF_ASKED ) {@@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,oid_array_clear (&commons);strbuf_release (&req_buf);strbuf_release (&cap_buf); +free (push_cert_nonce);
And this is my fault to forget freeing. Thanks for spotting and fixing.
Quoted 2 lines
send-pack.c +0
-0return ret; }
Re: [PATCH 11/22] gpg-interface: fix misdesigned signing key interfaces
In reply to Patrick SteinhardtQuoted 4 lines · Patrick Steinhardt
The interfaces to retrieve signing keys and their IDs are misdesigned as they return string constants even though they indeed allocate memory, which leads to memory leaks. Refactor the code to instead always return allocated strings and let the callers free them accordingly.
Makes sense.
Re: [PATCH 16/22] builtin/repack: fix leaking line buffer when packing promisors
In reply to Patrick SteinhardtQuoted 17 lines · Patrick Steinhardt
In
repack_promisor_objects()we read output from git-pack-objects(1) line by line, usingstrbuf_getline_lf(). We never free the line buffer, causing a memory leak. Plug it.This leak is being hit in t5616, but plugging it alone is not sufficient to make the whole test suite leak free.
Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/repack.c | 2 ++ 1 file changed, 2 insertions(+)builtin/repack.c +0
-0
builtin/repack.c static void repack_promisor_objects(const struct pack_objects_args *args, · line 433
Quoted 9 lines
builtin/repack.c +2
-0@@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,free (promisor_name); } +fclose (out);if (finish_command (&cmd))die (_ ("could not finish pack-objects to repack promisor objects" )); +strbuf_release (&line); }
Obviously correct.
Thanks.
Re: [PATCH 18/22] builtin/grep: fix leaking object context
In reply to Patrick SteinhardtQuoted 2 lines · Patrick Steinhardt
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
"tthe" -> "the".
Quoted 14 lines
call fails, leading to a memory leak. Plug it.
Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/grep.c | 1 + t/t6132-pathspec-exclude.sh | 1 + t/t6135-pathspec-with-attrs.sh | 2 ++ 3 files changed, 4 insertions(+)builtin/grep.c +0
-0
builtin/grep.c int cmd_grep(int argc, const char **argv, const char *prefix) · line 1138
Quoted 6 lines
builtin/grep.c +1
-0@@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) &oid, &oc)) {if (seen_dashdash)die (_ ("unable to resolve revision: %s" ), arg); +object_context_release (&oc);break ; }
OK. This is the "oh, this is not a revision argument" codepath. It is perfectly normal for get_oid_with_context() to fail here, and we should make sure we clear the context variable.
Thanks.
Re: [PATCH 20/22] match-trees: fix leaking prefixes in `shift_tree()`
In reply to Patrick SteinhardtQuoted 7 lines · Patrick Steinhardt
In
shift_tree()we allocate two empty strings that we end up passing tomatch_trees(). If that function finds a better match it will update these pointers to point to a newly allocated strings, freeing the old strings. We never free the final results though, neither the ones we have allocated ourselves, nor the one thatmatch_trees()might've returned to us.
Quoted 8 lines
Fix the resulting memory leaks by creating a common exit path where we free them.
Signed-off-by Patrick Steinhardt <[email protected]>--- match-trees.c | 10 +++++++--- t/t6409-merge-subtree.sh | 1 + 2 files changed, 8 insertions(+), 3 deletions(-)
We are not going to take the "best_match" out of this function, so somebody ought to free it, and that somebody must be this function.
Makes sense.
Re: [PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications
In reply to Patrick SteinhardtQuoted 12 lines · Patrick Steinhardt
There are two leaks in
apply_directory_rename_modifications():- We do not release the
dirs_to_insertstring list.- We do not release some
conflict_infowe put into the`opt->priv->paths` string map.The former is trivial to fix. The latter is a bit less straight forward:
theutilpointer of the string map may sometimes point to data that has been allocated viaCALLOC(), while at other times it may point to data that has been allocated via amem_pool.
Quoted 6 lines
It very much seems like an oversight that we didn't also allocate the conflict info in this code path via the memory pool, though. So let's fix that, which will also plug the memory leak for us.
Signed-off-by Patrick Steinhardt <[email protected]>---
May be nice if we can hear from the original author and the area expert.
Quoted 37 lines
merge-ort.c | 4 +++- t/t6423-merge-rename-directories.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)merge-ort.c +3
-1@@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,struct conflict_info *dir_ci;char *cur_dir = dirs_to_insert.items[i].string; -CALLOC_ARRAY (dir_ci,1 ); + dir_ci =mem_pool_calloc (&opt->priv->pool,1 ,sizeof (*dir_ci)); dir_ci->merged.directory_name = parent_name; len =strlen (parent_name);@@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,* Finally, record the new location. */ pair->two->path = new_path; + +string_list_clear (&dirs_to_insert,0 ); }/*** Function Grouping: functions related to regular rename detection ***/ t/t6423-merge-rename-directories.sh +1
-0@@ -25,6 +25,7 @@ test_description="recursive merge with directory renames" # underscore notation is to differentiate different # files that might be renamed into each other's paths.) +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh
Quoted 4 lines · Patrick Steinhardt
here's another round of memory leak fixes. This is the second-last round of "general" leak fixes -- so once this and the next series are merged, I have made a full pass over all failing test suites. There will still be a bunch of memory leaks, namely ~86 failing test suites.
I've read (or at least briefly scanned) most of the changes in this series over the course of several days, and did not spot anything glaringly wrong, other than a small typo or two whose fix should be trivial.
Thanks.
Elijah Newren
Reviewed-byRe: [PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications
In reply to Junio C HamanoQuoted 7 lines · Junio C Hamano
Quoted 4 lines · Patrick Steinhardt
There are two leaks in
apply_directory_rename_modifications():- We do not release the
dirs_to_insertstring list.
Quoted 7 lines
Quoted 7 lines
- We do not release some
conflict_infowe put into the`opt->priv->paths` string map.The former is trivial to fix. The latter is a bit less straight forward:
theutilpointer of the string map may sometimes point to data that has been allocated viaCALLOC(), while at other times it may point to data that has been allocated via amem_pool.
Oops.
Quoted 2 lines
Quoted 2 lines
It very much seems like an oversight that we didn't also allocate the conflict info in this code path via the memory pool, though.
Yep.
Quoted 42 lines
Quoted 4 lines
So let's fix that, which will also plug the memory leak for us.
Signed-off-by Patrick Steinhardt <[email protected]>---
May be nice if we can hear from the original author and the area expert.
Quoted 34 lines
merge-ort.c | 4 +++- t/t6423-merge-rename-directories.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)merge-ort.c +3
-1@@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,struct conflict_info *dir_ci;char *cur_dir = dirs_to_insert.items[i].string; -CALLOC_ARRAY (dir_ci,1 ); + dir_ci =mem_pool_calloc (&opt->priv->pool,1 ,sizeof (*dir_ci)); dir_ci->merged.directory_name = parent_name; len =strlen (parent_name);@@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,* Finally, record the new location. */ pair->two->path = new_path; + +string_list_clear (&dirs_to_insert,0 ); }/*** Function Grouping: functions related to regular rename detection ***/ t/t6423-merge-rename-directories.sh +0
-0@@ -25,6 +25,7 @@ test_description="recursive merge with directory renames" # underscore notation is to differentiate different # files that might be renamed into each other's paths.)
Quoted 3 lines
t/t6423-merge-rename-directories.sh test_description="recursive merge with directory renames" · line 30
Quoted 3 lines
t/t6423-merge-rename-directories.sh +1
-0+TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh
Looks good to me; thanks for fixing up my bugs.
v2 cover Memory leak fixes (pt.6)
In reply to Patrick SteinhardtHi,
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.
+ ## 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.
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
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 1/22 t/test-lib: allow skipping leak checks for passing tests
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". 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.
--- t/README | 3 +++ t/test-lib.sh | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-)
t/README +3 -0
@@ -386,6 +386,9 @@ 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 +10 -1
@@ -1558,8 +1558,16 @@ then passes_sanitize_leak=t fi - if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" then + if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" && + test -n "$passes_sanitize_leak" + then + skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing" + test_done + fi + sanitize_leak_check=t if test -n "$invert_exit_code" then
@@ -1597,6 +1605,7 @@ then export LSAN_OPTIONS elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" || + test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" || test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false then BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 2/22 fetch-pack: fix memory leaks on fetch negotiation
In reply to Patrick SteinhardtWe leak both the nt_object_array and negotiator structures in negotiate_using_fetch(). Plug both of these leaks.
These leaks were exposed by t5516, but fixing them is not sufficient to make the whole test suite leak free.
--- fetch-pack.c | 3 +++ 1 file changed, 3 insertions(+)
fetch-pack.c +3 -0
@@ -2227,7 +2227,10 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,trace2_region_leave ("fetch-pack" ,"negotiate_using_fetch" , the_repository);trace2_data_intmax ("negotiate_using_fetch" , the_repository,"total_rounds" , negotiation_round); +clear_common_flag (acked_commits); +object_array_clear (&nt_object_array); + negotiator.release (&negotiator);strbuf_release (&req_buf); }
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 3/22 send-pack: fix leaking common object IDs
In reply to Patrick SteinhardtWe're leaking the array of common object IDs in send_pack(). Fix this by creating a common exit path where we free the leaking data. While at it, unify some other cleanups now that we have a central place to put them.
--- send-pack.c | 34 ++++++++++++++++++++++------------ t/t5549-fetch-push-http.sh | 1 + 2 files changed, 23 insertions(+), 12 deletions(-)
send-pack.c +22 -12
@@ -508,7 +508,8 @@ int send_pack(struct send_pack_args *args,if (!remote_refs) {fprintf (stderr,"No refs in common and none specified; doing nothing.\n" "Perhaps you should specify a branch.\n" ); -return 0 ; + ret =0 ; + goto out; }git_config_get_bool ("push.negotiate" , &push_negotiate);
@@ -615,12 +616,11 @@ int send_pack(struct send_pack_args *args,* atomically, abort the whole operation. */ if (use_atomic) { -strbuf_release (&req_buf); -strbuf_release (&cap_buf);reject_atomic_push (remote_refs, args->send_mirror);error ("atomic push failed for ref %s. status: %d\n" , ref->name, ref->status); -return args->porcelain ?0 :-1 ; + ret = args->porcelain ?0 :-1 ; + goto out; }/* else fallthrough */ default :
@@ -682,8 +682,6 @@ int send_pack(struct send_pack_args *args,write_or_die (out, req_buf.buf, req_buf.len);packet_flush (out); } -strbuf_release (&req_buf); -strbuf_release (&cap_buf);if (use_sideband && cmds_sent) {memset (&demux,0 ,sizeof (demux));
@@ -721,7 +719,9 @@ int send_pack(struct send_pack_args *args,finish_async (&demux); } fd[1 ] =-1 ; -return -1 ; + + ret =-1 ; + goto out; }if (!args->stateless_rpc)/* Closed by pack_objects() via start_command() */
@@ -746,10 +746,12 @@ int send_pack(struct send_pack_args *args, }if (ret <0 ) -return ret; + goto out; -if (args->porcelain) -return 0 ; +if (args->porcelain) { + ret =0 ; + goto out; + }for (ref = remote_refs; ref; ref = ref->next) {switch (ref->status) {
@@ -758,8 +760,16 @@ int send_pack(struct send_pack_args *args,case REF_STATUS_OK :break ;default : -return -1 ; + ret =-1 ; + goto out; } } -return 0 ; + + ret =0 ; + +out: +oid_array_clear (&commons); +strbuf_release (&req_buf); +strbuf_release (&cap_buf); +return ret; }
t/t5549-fetch-push-http.sh +1 -0
@@ -5,6 +5,7 @@ test_description='fetch/push functionality using the HTTP protocol' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 4/22 builtin/push: fix leaking refspec query result
In reply to Patrick SteinhardtWhen appending a refspec via refspec_append_mapped() we leak the result of query_refspecs(). The overall logic around refspec queries is quite weird, as callers are expected to either set the src or dst pointers, and then the (allocated) result will be in the respective other struct member.
As we have the src member set, plugging the memory leak is thus as easy as just freeing the dst member. While at it, use designated initializers to initialize the structure.
This leak was exposed by t5516, but fixing it is not sufficient to make the whole test suite leak free.
--- builtin/push.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
builtin/push.c +5 -3
@@ -72,13 +72,15 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,const char *branch_name;if (remote->push.nr) { -struct refspec_item query; -memset (&query,0 ,sizeof (struct refspec_item )); - query.src = matched->name; +struct refspec_item query = { + .src = matched->name, + }; +if (!query_refspecs (&remote->push, &query) && query.dst) {refspec_appendf (refspec,"%s%s:%s" , query.force ?"+" :"" , query.src, query.dst); +free (query.dst);return ; } }
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 5/22 upload-pack: fix leaking child process data on reachability checks
In reply to Patrick SteinhardtWe spawn a git-rev-list(1) command to perform reachability checks in "upload-pack.c". We do not release memory associated with the process in error cases though, thus leaking memory.
Fix these by calling child_process_clear().
--- t/t5516-fetch-push.sh | 1 + upload-pack.c | 22 ++++++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-)
t/t5516-fetch-push.sh +1 -0
@@ -19,6 +19,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh D=$(pwd)
upload-pack.c +16 -6
@@ -709,10 +709,13 @@ static int get_reachable_list(struct upload_pack_data *data,struct object *o;char namebuf[GIT_MAX_HEXSZ +2 ];/* ^ + hash + LF */ const unsigned hexsz = the_hash_algo->hexsz; +int ret;if (do_reachable_revlist (&cmd, &data->shallows, reachable, - data->allow_uor) <0 ) -return -1 ; + data->allow_uor) <0 ) { + ret =-1 ; + goto out; + }while ((i =read_in_full (cmd.out, namebuf, hexsz +1 )) == hexsz +1 ) {struct object_id oid;
@@ -736,10 +739,16 @@ static int get_reachable_list(struct upload_pack_data *data, }close (cmd.out); -if (finish_command (&cmd)) -return -1 ; +if (finish_command (&cmd)) { + ret =-1 ; + goto out; + } -return 0 ; + ret =0 ; + +out: +child_process_clear (&cmd); +return ret; }static int has_unreachable (struct object_array *src,enum allow_uor allow_uor)
@@ -749,7 +758,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)int i;if (do_reachable_revlist (&cmd, src,NULL , allow_uor) <0 ) -return 1 ; + goto error;/* * The commits out of the rev-list are not ancestors of
@@ -775,6 +784,7 @@ static int has_unreachable(struct object_array *src, enum allow_uor allow_uor) error:if (cmd.out >=0 )close (cmd.out); +child_process_clear (&cmd);return 1 ; }
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 6/22 submodule: fix leaking fetch task data
In reply to Patrick SteinhardtThe submodule_parallel_fetch structure contains various data structures that we use to set up parallel fetches of submodules. We do not free some of its data though, causing memory leaks. Plug those.
--- submodule.c | 2 ++ t/t5526-fetch-submodules.sh | 1 + 2 files changed, 3 insertions(+)
submodule.c +2 -0
@@ -1883,6 +1883,8 @@ int fetch_submodules(struct repository *r, out:free_submodules_data (&spf.changed_submodule_names);string_list_clear (&spf.seen_submodule_names,0 ); +strbuf_release (&spf.submodules_with_errors); +free (spf.oid_fetch_tasks);return spf.result; }
t/t5526-fetch-submodules.sh +1 -0
@@ -6,6 +6,7 @@ test_description='Recursive "git fetch" for submodules' GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh pwd=$(pwd)
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 7/22 builtin/submodule--helper: fix leaking refs on push-check
In reply to Patrick SteinhardtIn the push-check subcommand of the submodule helper we acquire a list of local refs, but never free that list. Fix this memory leak.
--- builtin/submodule--helper.c | 2 ++ t/t5531-deep-submodule-push.sh | 1 + 2 files changed, 3 insertions(+)
builtin/submodule--helper.c +2 -0
@@ -2958,7 +2958,9 @@ static int push_check(int argc, const char **argv, const char *prefix UNUSED) rs->src); } } +refspec_clear (&refspec); +free_refs (local_refs); }free (head);
t/t5531-deep-submodule-push.sh +1 -0
@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup '
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 8/22 remote: fix leaking tracking refs
In reply to Patrick SteinhardtWhen computing the remote tracking ref we cause two memory leaks:
- We leak when remote_tracking() fails.
- We leak when the call to remote_tracking() succeeds and sets
`ref->tracking_ref()`.
Fix both of these leaks.
--- remote.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
remote.c +4 -1
@@ -1123,6 +1123,7 @@ void free_one_ref(struct ref *ref)return ;free_one_ref (ref->peer_ref);free (ref->remote_status); +free (ref->tracking_ref);free (ref->symref);free (ref); }
@@ -2620,8 +2621,10 @@ static int remote_tracking(struct remote *remote, const char *refname, dst =apply_refspecs (&remote->fetch, refname);if (!dst)return -1 ;/* no tracking ref for refname at remote */ -if (refs_read_ref (get_main_ref_store (the_repository), dst, oid)) +if (refs_read_ref (get_main_ref_store (the_repository), dst, oid)) { +free (dst);return -1 ;/* we know what the tracking ref is but we cannot read it */ + } *dst_refname = dst;return 0 ;
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 9/22 remote: fix leak in reachability check of a remote-tracking ref
In reply to Patrick SteinhardtIn check_if_includes_upstream() we retrieve the local ref corresponding to a remote-tracking ref we want to check reachability for. We never free that local ref and thus cause a memory leak. Fix this.
--- remote.c | 1 + t/t5533-push-cas.sh | 1 + 2 files changed, 2 insertions(+)
remote.c +1 -0
@@ -2774,6 +2774,7 @@ static void check_if_includes_upstream(struct ref *remote)if (is_reachable_in_reflog (local->name, remote) <=0 ) remote->unreachable =1 ; +free_one_ref (local); } static void apply_cas(struct push_cas_option *cas,
t/t5533-push-cas.sh +1 -0
@@ -5,6 +5,7 @@ test_description='compare & swap push force/delete safety' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh setup_srcdst_basic () {
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 10/22 send-pack: fix leaking push cert nonce
In reply to Patrick SteinhardtWhen 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 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 without having to cast away its constness.
This leak was exposed by t5534, but fixing it is not sufficient to make the whole test suite leak free.
--- send-pack.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
send-pack.c +7 -5
@@ -501,7 +501,7 @@ int send_pack(struct send_pack_args *args,unsigned cmds_sent =0 ;int ret;struct async demux; -const char *push_cert_nonce =NULL ; +char *push_cert_nonce =NULL ;struct packet_reader reader;int use_bitmaps;
@@ -550,10 +550,11 @@ int send_pack(struct send_pack_args *args,if (args->push_cert !=SEND_PACK_PUSH_CERT_NEVER ) {size_t len; - push_cert_nonce =server_feature_value ("push-cert" , &len); -if (push_cert_nonce) { -reject_invalid_nonce (push_cert_nonce, len); - push_cert_nonce =xmemdupz (push_cert_nonce, len); +const char *nonce =server_feature_value ("push-cert" , &len); + +if (nonce) { +reject_invalid_nonce (nonce, len); + push_cert_nonce =xmemdupz (nonce, len); }else if (args->push_cert ==SEND_PACK_PUSH_CERT_ALWAYS ) {die (_ ("the receiving end does not support --signed push" )); }else if (args->push_cert ==SEND_PACK_PUSH_CERT_IF_ASKED ) {
@@ -771,5 +772,6 @@ int send_pack(struct send_pack_args *args,oid_array_clear (&commons);strbuf_release (&req_buf);strbuf_release (&cap_buf); +free (push_cert_nonce);return ret; }
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 11/22 gpg-interface: fix misdesigned signing key interfaces
In reply to Patrick SteinhardtThe interfaces to retrieve signing keys and their IDs are misdesigned as they return string constants even though they indeed allocate memory, which leads to memory leaks. Refactor the code to instead always return allocated strings and let the callers free them accordingly.
--- builtin/tag.c | 3 ++- commit.c | 9 ++++++--- gpg-interface.c | 26 +++++++++++++++----------- gpg-interface.h | 4 ++-- send-pack.c | 6 ++++-- t/t5534-push-signed.sh | 1 + 6 files changed, 30 insertions(+), 19 deletions(-)
builtin/tag.c +2 -1
@@ -160,7 +160,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,const struct git_hash_algo *compat = the_repository->compat_hash_algo;struct strbuf sig =STRBUF_INIT , compat_sig =STRBUF_INIT ;struct strbuf compat_buf =STRBUF_INIT ; -const char *keyid =get_signing_key (); +char *keyid =get_signing_key ();int ret =-1 ;if (sign_buffer (buffer, &sig, keyid))
@@ -190,6 +190,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid,strbuf_release (&sig);strbuf_release (&compat_sig);strbuf_release (&compat_buf); +free (keyid);return ret; }
commit.c +6 -3
@@ -1150,11 +1150,14 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gistatic int sign_commit_to_strbuf (struct strbuf *sig,struct strbuf *buf,const char *keyid) { +char *keyid_to_free =NULL ; +int ret =0 ;if (!keyid || !*keyid) - keyid =get_signing_key (); + keyid = keyid_to_free =get_signing_key ();if (sign_buffer (buf, sig, keyid)) -return -1 ; -return 0 ; + ret =-1 ; +free (keyid_to_free); +return ret; } int parse_signed_commit(const struct commit *commit,
gpg-interface.c +15 -11
@@ -45,8 +45,8 @@ struct gpg_format { size_t signature_size);int (*sign_buffer)(struct strbuf *buffer,struct strbuf *signature,const char *signing_key); -const char *(*get_default_key)(void ); -const char *(*get_key_id)(void ); +char *(*get_default_key)(void ); +char *(*get_key_id)(void ); };static const char *openpgp_verify_args[] = {
@@ -86,9 +86,9 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,static int sign_buffer_ssh (struct strbuf *buffer,struct strbuf *signature,const char *signing_key); -static const char *get_default_ssh_signing_key (void ); +static char *get_default_ssh_signing_key (void ); -static const char *get_ssh_key_id (void ); +static char *get_ssh_key_id (void );static struct gpg_format gpg_format[] = { {
@@ -847,7 +847,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key) }/* Returns the first public key from an ssh-agent to use for signing */ -static const char *get_default_ssh_signing_key (void ) +static char *get_default_ssh_signing_key (void ) {struct child_process ssh_default_key =CHILD_PROCESS_INIT ;int ret =-1 ;
@@ -899,12 +899,16 @@ static const char *get_default_ssh_signing_key(void)return default_key; } -static const char *get_ssh_key_id (void ) { -return get_ssh_key_fingerprint (get_signing_key ()); +static char *get_ssh_key_id (void ) +{ +char *signing_key =get_signing_key (); +char *key_id =get_ssh_key_fingerprint (signing_key); +free (signing_key); +return key_id; }/* Returns a textual but unique representation of the signing key */ -const char *get_signing_key_id (void ) +char *get_signing_key_id (void ) {gpg_interface_lazy_init ();
@@ -916,17 +920,17 @@ const char *get_signing_key_id(void)return get_signing_key (); } -const char *get_signing_key (void ) +char *get_signing_key (void ) {gpg_interface_lazy_init ();if (configured_signing_key) -return configured_signing_key; +return xstrdup (configured_signing_key);if (use_format->get_default_key) {return use_format->get_default_key (); } -return git_committer_info (IDENT_STRICT |IDENT_NO_DATE ); +return xstrdup (git_committer_info (IDENT_STRICT |IDENT_NO_DATE )); }const char *gpg_trust_level_to_str (enum signature_trust_level level)
gpg-interface.h +2 -2
@@ -80,13 +80,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,const char *gpg_trust_level_to_str (enum signature_trust_level level);void set_signing_key (const char *); -const char *get_signing_key (void ); +char *get_signing_key (void );/* * Returns a textual unique representation of the signing key in use * Either a GPG KeyID or a SSH Key Fingerprint */ -const char *get_signing_key_id (void ); +char *get_signing_key_id (void );int check_signature (struct signature_check *sigc,const char *signature,size_t slen); void print_signature_buffer(const struct signature_check *sigc,
send-pack.c +4 -2
@@ -348,7 +348,8 @@ static int generate_push_cert(struct strbuf *req_buf, {const struct ref *ref;struct string_list_item *item; -char *signing_key_id =xstrdup (get_signing_key_id ()); +char *signing_key_id =get_signing_key_id (); +char *signing_key =get_signing_key ();const char *cp, *np;struct strbuf cert =STRBUF_INIT ;int update_seen =0 ;
@@ -381,7 +382,7 @@ static int generate_push_cert(struct strbuf *req_buf,if (!update_seen) goto free_return; -if (sign_buffer (&cert, &cert,get_signing_key ())) +if (sign_buffer (&cert, &cert, signing_key))die (_ ("failed to sign the push certificate" ));packet_buf_write (req_buf,"push-cert%c%s" ,0 , cap_string);
@@ -394,6 +395,7 @@ static int generate_push_cert(struct strbuf *req_buf, free_return:free (signing_key_id); +free (signing_key);strbuf_release (&cert);return update_seen; }
t/t5534-push-signed.sh +1 -0
@@ -5,6 +5,7 @@ test_description='signed push' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-gpg.sh
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 12/22 object: clear grafts when clearing parsed object pool
In reply to Patrick SteinhardtWe do not clear grafts part of the parsed object pool when clearing the pool itself, which can lead to memory leaks when a repository is being cleared.
Fix this by moving reset_commit_grafts() into "object.c" and making it part of the struct parsed_object_pool interface such that we can call it from parsed_object_pool_clear(). Adapt parsed_object_pool_new() to take and store a reference to its owning repository, which is needed by unparse_commit().
--- commit.c | 14 +------------- commit.h | 3 ++- object.c | 14 +++++++++++++- object.h | 4 +++- repository.c | 2 +- shallow.c | 2 +- 6 files changed, 21 insertions(+), 18 deletions(-)
commit.c +1 -13
@@ -177,7 +177,7 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid) commit_graft_oid_access); } -static void unparse_commit (struct repository *r,const struct object_id *oid) +void unparse_commit (struct repository *r,const struct object_id *oid) {struct commit *c =lookup_commit (r, oid);
@@ -318,18 +318,6 @@ int for_each_commit_graft(each_commit_graft_fn fn, void *cb_data)return ret; } -void reset_commit_grafts (struct repository *r) -{ -int i; - -for (i =0 ; i < r->parsed_objects->grafts_nr; i++) { -unparse_commit (r, &r->parsed_objects->grafts[i]->oid); -free (r->parsed_objects->grafts[i]); - } - r->parsed_objects->grafts_nr =0 ; - r->parsed_objects->commit_graft_prepared =0 ; -} -struct commit_buffer {void *buffer;unsigned long size;
commit.h +2 -1
@@ -108,6 +108,8 @@ static inline int repo_parse_commit_no_graph(struct repository *r,void parse_commit_or_die (struct commit *item); +void unparse_commit (struct repository *r,const struct object_id *oid); +struct buffer_slab ;struct buffer_slab *allocate_commit_buffer_slab (void );void free_commit_buffer_slab (struct buffer_slab *bs);
@@ -240,7 +242,6 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid);int register_commit_graft (struct repository *r,struct commit_graft *,int );void prepare_commit_graft (struct repository *r);struct commit_graft *lookup_commit_graft (struct repository *r,const struct object_id *oid); -void reset_commit_grafts (struct repository *r);struct commit *get_fork_point (const char *refname,struct commit *commit);
object.c +13 -1
@@ -545,11 +545,12 @@ void repo_clear_commit_marks(struct repository *r, unsigned int flags) } } -struct parsed_object_pool *parsed_object_pool_new (void ) +struct parsed_object_pool *parsed_object_pool_new (struct repository *repo) {struct parsed_object_pool *o =xmalloc (sizeof (*o));memset (o,0 ,sizeof (*o)); + o->repo = repo; o->blob_state =allocate_alloc_state (); o->tree_state =allocate_alloc_state (); o->commit_state =allocate_alloc_state ();
@@ -628,6 +629,16 @@ void raw_object_store_clear(struct raw_object_store *o)hashmap_clear (&o->pack_map); } +void parsed_object_pool_reset_commit_grafts (struct parsed_object_pool *o) +{ +for (int i =0 ; i < o->grafts_nr; i++) { +unparse_commit (o->repo, &o->grafts[i]->oid); +free (o->grafts[i]); + } + o->grafts_nr =0 ; + o->commit_graft_prepared =0 ; +} +void parsed_object_pool_clear (struct parsed_object_pool *o) {/*
@@ -659,6 +670,7 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)free_commit_buffer_slab (o->buffer_slab); o->buffer_slab =NULL ; +parsed_object_pool_reset_commit_grafts (o);clear_alloc_state (o->blob_state);clear_alloc_state (o->tree_state);clear_alloc_state (o->commit_state);
object.h +3 -1
@@ -7,6 +7,7 @@ struct buffer_slab;struct repository ;struct parsed_object_pool { +struct repository *repo;struct object **obj_hash;int nr_objs, obj_hash_size;
@@ -31,8 +32,9 @@ struct parsed_object_pool {struct buffer_slab *buffer_slab; }; -struct parsed_object_pool *parsed_object_pool_new (void ); +struct parsed_object_pool *parsed_object_pool_new (struct repository *repo);void parsed_object_pool_clear (struct parsed_object_pool *o); +void parsed_object_pool_reset_commit_grafts (struct parsed_object_pool *o);struct object_list {struct object *item;
repository.c +1 -1
@@ -54,7 +54,7 @@ void initialize_repository(struct repository *repo) { repo->objects =raw_object_store_new (); repo->remote_state =remote_state_new (); - repo->parsed_objects =parsed_object_pool_new (); + repo->parsed_objects =parsed_object_pool_new (repo);ALLOC_ARRAY (repo->index,1 );index_state_init (repo->index, repo);
shallow.c +1 -1
@@ -97,7 +97,7 @@ static void reset_repository_shallow(struct repository *r) { r->parsed_objects->is_shallow =-1 ;stat_validity_clear (r->parsed_objects->shallow_stat); -reset_commit_grafts (r); +parsed_object_pool_reset_commit_grafts (r->parsed_objects); }int commit_shallow_file (struct repository *r,struct shallow_lock *lk)
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 13/22 shallow: free grafts when unregistering them
In reply to Patrick SteinhardtWhen removing a graft via unregister_shallow() we remove it from the grafts array, but do not free the structure. Fix this to plug the leak.
--- shallow.c | 4 +++- t/t5537-fetch-shallow.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
shallow.c +3 -1
@@ -51,10 +51,12 @@ int unregister_shallow(const struct object_id *oid)int pos =commit_graft_pos (the_repository, oid);if (pos <0 )return -1 ; -if (pos +1 < the_repository->parsed_objects->grafts_nr) +if (pos +1 < the_repository->parsed_objects->grafts_nr) { +free (the_repository->parsed_objects->grafts[pos]);MOVE_ARRAY (the_repository->parsed_objects->grafts + pos, the_repository->parsed_objects->grafts + pos +1 , the_repository->parsed_objects->grafts_nr - pos -1 ); + } the_repository->parsed_objects->grafts_nr--;return 0 ; }
t/t5537-fetch-shallow.sh +1 -0
@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh commit() {
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 14/22 shallow: fix leaking members of `struct shallow_info`
In reply to Patrick SteinhardtWe do not free several struct members in clear_shallow_info(). Fix this to plug the resulting leaks.
--- shallow.c | 9 +++++++++ t/t5538-push-shallow.sh | 1 + 2 files changed, 10 insertions(+)
shallow.c +9 -0
@@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)void clear_shallow_info (struct shallow_info *info) { +if (info->used_shallow) { +for (size_t i =0 ; i < info->shallow->nr; i++) +free (info->used_shallow[i]); +free (info->used_shallow); + } + +free (info->need_reachability_test); +free (info->reachable); +free (info->shallow_ref);free (info->ours);free (info->theirs); }
t/t5538-push-shallow.sh +1 -0
@@ -5,6 +5,7 @@ test_description='push from/to a shallow clone' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh commit() {
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 15/22 negotiator/skipping: fix leaking commit entries
In reply to Patrick SteinhardtWhen releasing the skipping negotiator we free its priority queue, but not the contained entries. Fix this to plug a memory leak.
--- negotiator/skipping.c | 7 +++++-- t/t5552-skipping-fetch-negotiator.sh | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-)
negotiator/skipping.c +5 -2
@@ -247,8 +247,11 @@ static int ack(struct fetch_negotiator *n, struct commit *c)static void release (struct fetch_negotiator *n) { -clear_prio_queue (&((struct data *)n->data)->rev_list); -FREE_AND_NULL (n->data); +struct data *data = n->data; +for (int i =0 ; i < data->rev_list.nr; i++) +free (data->rev_list.array[i].data); +clear_prio_queue (&data->rev_list); +FREE_AND_NULL (data); }void skipping_negotiator_init (struct fetch_negotiator *negotiator)
t/t5552-skipping-fetch-negotiator.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='test skipping fetch negotiator' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'fetch.negotiationalgorithm config' '
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 16/22 builtin/repack: fix leaking line buffer when packing promisors
In reply to Patrick SteinhardtIn repack_promisor_objects() we read output from git-pack-objects(1) line by line, using strbuf_getline_lf(). We never free the line buffer, causing a memory leak. Plug it.
This leak is being hit in t5616, but plugging it alone is not sufficient to make the whole test suite leak free.
--- builtin/repack.c | 2 ++ 1 file changed, 2 insertions(+)
builtin/repack.c +2 -0
@@ -425,9 +425,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,free (promisor_name); } +fclose (out);if (finish_command (&cmd))die (_ ("could not finish pack-objects to repack promisor objects" )); +strbuf_release (&line); }struct pack_geometry {
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 17/22 builtin/pack-objects: plug leaking list of keep-packs
In reply to Patrick SteinhardtThe --keep-pack option of git-pack-objects(1) populates the arguments into a string list. And while the list is marked as NODUP and thus won't duplicate the strings, the list entries themselves still need to be free'd. We don't though, causing a leak.
Plug it.
--- builtin/pack-objects.c | 1 + t/t5616-partial-clone.sh | 1 + 2 files changed, 2 insertions(+)
builtin/pack-objects.c +1 -0
@@ -4641,6 +4641,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) cleanup:clear_packing_data (&to_pack);list_objects_filter_release (&filter_options); +string_list_clear (&keep_pack_list,0 );strvec_clear (&rp);return 0 ;
t/t5616-partial-clone.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git partial clone' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # create a normal "src" repo where we can later create new commits.
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 18/22 builtin/grep: fix leaking object context
In reply to Patrick SteinhardtEven when get_oid_with_context() fails it may have allocated some data 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.
--- builtin/grep.c | 1 + t/t6132-pathspec-exclude.sh | 1 + t/t6135-pathspec-with-attrs.sh | 2 ++ 3 files changed, 4 insertions(+)
builtin/grep.c +1 -0
@@ -1133,6 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) &oid, &oc)) {if (seen_dashdash)die (_ ("unable to resolve revision: %s" ), arg); +object_context_release (&oc);break ; }
t/t6132-pathspec-exclude.sh +1 -0
@@ -2,6 +2,7 @@ test_description='test case exclude pathspec' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
t/t6135-pathspec-with-attrs.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='test labels in pathspecs' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup a tree' '
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 19/22 builtin/fmt-merge-msg: fix leaking buffers
In reply to Patrick SteinhardtFix leaking input and output buffers in git-fmt-merge-msg(1).
--- builtin/fmt-merge-msg.c | 2 ++ t/t6200-fmt-merge-msg.sh | 1 + 2 files changed, 3 insertions(+)
builtin/fmt-merge-msg.c +2 -0
@@ -67,6 +67,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)return ret;write_in_full (STDOUT_FILENO , output.buf, output.len); +strbuf_release (&input); +strbuf_release (&output);free (inpath);return 0 ; }
t/t6200-fmt-merge-msg.sh +1 -0
@@ -8,6 +8,7 @@ test_description='fmt-merge-msg test' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-gpg.sh"
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 20/22 match-trees: fix leaking prefixes in `shift_tree()`
In reply to Patrick SteinhardtIn shift_tree() we allocate two empty strings that we end up passing to match_trees(). If that function finds a better match it will update these pointers to point to a newly allocated strings, freeing the old strings. We never free the final results though, neither the ones we have allocated ourselves, nor the one that match_trees() might've returned to us.
Fix the resulting memory leaks by creating a common exit path where we free them.
--- match-trees.c | 10 +++++++--- t/t6409-merge-subtree.sh | 1 + 2 files changed, 8 insertions(+), 3 deletions(-)
match-trees.c +7 -3
@@ -294,18 +294,22 @@ void shift_tree(struct repository *r,unsigned short mode;if (!*del_prefix) -return ; + goto out;if (get_tree_entry (r, hash2, del_prefix, shifted, &mode))die ("cannot find path %s in tree %s" , del_prefix,oid_to_hex (hash2)); -return ; + goto out; }if (!*add_prefix) -return ; + goto out;splice_tree (hash1, add_prefix, hash2, shifted); + +out: +free (add_prefix); +free (del_prefix); }/*
t/t6409-merge-subtree.sh +1 -0
@@ -5,6 +5,7 @@ test_description='subtree merge strategy' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup '
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 21/22 merge-ort: fix two leaks when handling directory rename modifications
In reply to Patrick SteinhardtThere are two leaks in apply_directory_rename_modifications():
- We do not release the dirs_to_insert string list.
- We do not release some conflict_info we put into the
`opt->priv->paths` string map.
The former is trivial to fix. The latter is a bit less straight forward:
the util pointer of the string map may sometimes point to data that has been allocated via CALLOC(), while at other times it may point to data that has been allocated via a mem_pool.
It very much seems like an oversight that we didn't also allocate the conflict info in this code path via the memory pool, though. So let's fix that, which will also plug the memory leak for us.
--- merge-ort.c | 4 +++- t/t6423-merge-rename-directories.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
merge-ort.c +3 -1
@@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,struct conflict_info *dir_ci;char *cur_dir = dirs_to_insert.items[i].string; -CALLOC_ARRAY (dir_ci,1 ); + dir_ci =mem_pool_calloc (&opt->priv->pool,1 ,sizeof (*dir_ci)); dir_ci->merged.directory_name = parent_name; len =strlen (parent_name);
@@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt,* Finally, record the new location. */ pair->two->path = new_path; + +string_list_clear (&dirs_to_insert,0 ); }/*** Function Grouping: functions related to regular rename detection ***/
t/t6423-merge-rename-directories.sh +1 -0
@@ -25,6 +25,7 @@ test_description="recursive merge with directory renames" # underscore notation is to differentiate different # files that might be renamed into each other's paths.) +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
PATCH v2 22/22 builtin/repack: fix leaking keep-pack list
In reply to Patrick SteinhardtThe list of packs to keep is populated via a command line option but never free'd. Plug this memory leak.
--- builtin/repack.c | 1 + t/t6500-gc.sh | 1 + t/t7703-repack-geometric.sh | 1 + 3 files changed, 3 insertions(+)
builtin/repack.c +1 -0
@@ -1543,6 +1543,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) } cleanup: +string_list_clear (&keep_pack_list,0 );string_list_clear (&names,1 );existing_packs_release (&existing);free_pack_geometry (&geometry);
t/t6500-gc.sh +1 -0
@@ -3,6 +3,7 @@ test_description='basic git gc tests ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh
t/t7703-repack-geometric.sh +1 -0
@@ -2,6 +2,7 @@ test_description='git repack --geometric works correctly' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_TEST_MULTI_PACK_INDEX=0
Signature · 1 line
2.46.0.519.g2e7b89e038.dirty
Quoted 6 lines · Patrick Steinhardt
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-failingvalue 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.
Quoted 4 lines · Patrick Steinhardt
Hi,
this is the second version of this round of memory leak fixes.
Quoted 8 lines
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-failingvalue in t/README.- Some more trivial commit message improvements.
Thanks!
Looking good. Let me mark the topic for 'next' soonish.
Thanks.