| Message ID | cover.1730786195.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Memory leak fixes (pt.9) | expand |
Patrick Steinhardt <ps@pks.im> writes: > Changes compared to v2: > > - Remove an unnecessary cast. > > - Fix a duplicate newline. > > - Polish a couple of commit messages. > > Thanks! I spot checked the ones that did not change from v2 and the ones I checked at all looked sensible. Perhaps this is now ready for 'next'? Thanks.
On Mon, Nov 04, 2024 at 10:51:10PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Changes compared to v2: > > > > - Remove an unnecessary cast. > > > > - Fix a duplicate newline. > > > > - Polish a couple of commit messages. > > > > Thanks! > > I spot checked the ones that did not change from v2 and the ones I > checked at all looked sensible. Perhaps this is now ready for > 'next'? From my point of view it should be ready, yes. Patrick
On 24/11/05 07:52AM, Patrick Steinhardt wrote: > On Mon, Nov 04, 2024 at 10:51:10PM -0800, Junio C Hamano wrote: > > Patrick Steinhardt <ps@pks.im> writes: > > > > > Changes compared to v2: > > > > > > - Remove an unnecessary cast. > > > > > > - Fix a duplicate newline. > > > > > > - Polish a couple of commit messages. > > > > > > Thanks! > > > > I spot checked the ones that did not change from v2 and the ones I > > checked at all looked sensible. Perhaps this is now ready for > > 'next'? > > From my point of view it should be ready, yes. From looking at the range-diff, this version looks good to me. :) -Justin
Hi, this is the third revision of my 9th series of memory leak fixes. Changes compared to v2: - Remove an unnecessary cast. - Fix a duplicate newline. - Polish a couple of commit messages. Thanks! Patrick Patrick Steinhardt (22): builtin/ls-remote: plug leaking server options t/helper: fix leaks in "reach" test tool grep: fix leak in `grep_splice_or()` builtin/grep: fix leak with `--max-count=0` revision: fix leaking bloom filters diff-lib: fix leaking diffopts in `do_diff_cache()` pretty: clear signature check upload-pack: fix leaking URI protocols builtin/commit: fix leaking change data contents trailer: fix leaking trailer values trailer: fix leaking strbufs when formatting trailers builtin/commit: fix leaking cleanup config transport-helper: fix leaking import/export marks builtin/tag: fix leaking key ID on failure to sign combine-diff: fix leaking lost lines dir: release untracked cache data sparse-index: correctly free EWAH contents t/helper: stop re-initialization of `the_repository` t/helper: fix leaking buffer in "dump-untracked-cache" dir: fix leak when parsing "status.showUntrackedFiles" builtin/merge: release output buffer after performing merge list-objects-filter-options: work around reported leak on error builtin/commit.c | 26 +++++++++++++++++------ builtin/grep.c | 13 +++++++++--- builtin/ls-remote.c | 1 + builtin/merge.c | 1 + builtin/tag.c | 2 +- combine-diff.c | 5 +++-- diff-lib.c | 1 + dir.c | 12 +++++++++-- grep.c | 1 + list-objects-filter-options.c | 17 ++++++--------- pretty.c | 1 + revision.c | 5 +++++ sparse-index.c | 7 ++++-- t/helper/test-dump-untracked-cache.c | 2 ++ t/helper/test-reach.c | 10 +++++++++ t/helper/test-read-cache.c | 2 -- t/t4038-diff-combined.sh | 1 + t/t4202-log.sh | 1 + t/t4216-log-bloom.sh | 1 + t/t5702-protocol-v2.sh | 1 + t/t5801-remote-helpers.sh | 1 + t/t6112-rev-list-filters-objects.sh | 1 + t/t6424-merge-unrelated-index-changes.sh | 1 + t/t6600-test-reach.sh | 1 + t/t7004-tag.sh | 1 + t/t7031-verify-tag-signed-ssh.sh | 1 + t/t7063-status-untracked-cache.sh | 1 + t/t7500-commit-template-squash-signoff.sh | 1 + t/t7502-commit-porcelain.sh | 1 + t/t7510-signed-commit.sh | 1 + t/t7513-interpret-trailers.sh | 1 + t/t7519-status-fsmonitor.sh | 1 + t/t7528-signed-commit-ssh.sh | 1 + t/t7610-mergetool.sh | 1 + t/t7810-grep.sh | 1 + trailer.c | 22 +++++++++++++------ transport-helper.c | 2 ++ upload-pack.c | 1 + 38 files changed, 115 insertions(+), 35 deletions(-) Range-diff against v2: 1: 89b66411354 ! 1: fbb9e68e2f2 builtin/ls-remote: plug leaking server options @@ Metadata ## Commit message ## builtin/ls-remote: plug leaking server options - The server options populated via `OPT_STRING_LIST()` is never cleared, - causing a memory leak. Plug it. + The list of server options populated via `OPT_STRING_LIST()` is never + cleared, causing a memory leak. Plug it. This leak is exposed by t5702, but plugging it alone does not make the whole test suite pass. 2: 1c42e194b20 = 2: 17136f4bdb3 t/helper: fix leaks in "reach" test tool 3: cb4eee37b40 ! 3: 74b21470194 grep: fix leak in `grep_splice_or()` @@ Commit message grep: fix leak in `grep_splice_or()` In `grep_splice_or()` we search for the next `TRUE` node in our tree of - grep exrpessions and replace it with the given new expression. But we + grep expressions and replace it with the given new expression. But we don't free the old node, which causes a memory leak. Plug it. This leak is exposed by t7810, but plugging it alone isn't sufficient to 4: 6b2c8842ef5 = 4: d716f93169a builtin/grep: fix leak with `--max-count=0` 5: 7527b31a28f = 5: aeb8a19d28d revision: fix leaking bloom filters 6: 60af98cb2c7 ! 6: 24d9d9b1358 diff-lib: fix leaking diffopts in `do_diff_cache()` @@ Commit message In `do_diff_cache()` we initialize a new `rev_info` and then overwrite its `diffopt` with a user-provided set of options. This can leak memory because `repo_init_revisions()` may end up allocating memory for the - `diffopt` itself depending on the configuration. And as that field is - overwritten we won't ever free that. + `diffopt` itself depending on the configuration. And since that field is + overwritten we won't ever free it. Plug the memory leak by releasing the diffopts before we overwrite them. 7: 5d5f6867f91 ! 7: 58ebef7e757 pretty: clear signature check @@ Metadata ## Commit message ## pretty: clear signature check - The signature check in of the formatting context is never getting - released. Fix this to plug the resulting memory leak. + The signature check in the formatting context is never getting released. + Fix this to plug the resulting memory leak. Signed-off-by: Patrick Steinhardt <ps@pks.im> 8: 0d503e40194 = 8: c5813079d44 upload-pack: fix leaking URI protocols 9: 9f967dfe5d5 = 9: f66fa4e0e25 builtin/commit: fix leaking change data contents 10: e3ffd59123f ! 10: dac63bae39e trailer: fix leaking trailer values @@ trailer.c: static char *apply_command(struct conf_info *conf, const char *arg) + char *arg; + if (arg_tok->value && arg_tok->value[0]) { -- arg = arg_tok->value; -+ arg = (char *)arg_tok->value; + arg = arg_tok->value; } else { - if (in_tok && in_tok->value) +@@ trailer.c: static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg arg = xstrdup(in_tok->value); else arg = xstrdup(""); 11: 5b851453bce ! 11: 82269e5d5be trailer: fix leaking strbufs when formatting trailers @@ Metadata ## Commit message ## trailer: fix leaking strbufs when formatting trailers - We are populating, but never releasing two string buffers in - `format_trailers()`, causing a memory leak. Plug this leak by lifting - those buffers outside of the loop and releasing them on function return. - This fixes the memory leaks, but also optimizes the loop as we don't - have to reallocate the buffers on every single iteration. + When formatting trailer lines we iterate through each of the trailers + and munge their respective token/value pairs according to the trailer + options. When formatting a trailer that has its `item->token` pointer + set we perform the munging in two local buffers. In the case where we + figure out that the value is empty and `trim_empty` is set we just skip + over the trailer item. But the buffers are local to the loop and we + don't release their contents, leading to a memory leak. + + Plug this leak by lifting the buffers outside of the loop and releasing + them on function return. This fixes the memory leaks, but also optimizes + the loop as we don't have to reallocate the buffers on every single + iteration. Signed-off-by: Patrick Steinhardt <ps@pks.im> @@ trailer.c: void format_trailers(const struct process_trailer_options *opts, size_t origlen = out->len; struct list_head *pos; struct trailer_item *item; - -+ +@@ trailer.c: void format_trailers(const struct process_trailer_options *opts, list_for_each(pos, trailers) { item = list_entry(pos, struct trailer_item, list); if (item->token) { 12: 60c3f6146f3 = 12: a627e03702e builtin/commit: fix leaking cleanup config 13: ea81cd8db86 = 13: 40e0c2a2cac transport-helper: fix leaking import/export marks 14: b700ab9079f = 14: ffa5d9eae7e builtin/tag: fix leaking key ID on failure to sign 15: 76bbcb3fe30 ! 15: 70dd0cb6933 combine-diff: fix leaking lost lines @@ Commit message The `cnt` variable tracks the number of lines in a patch diff. It can happen though that there are no newlines, in which case we'd still end up allocating our array of `sline`s. In fact, we always allocate it with - `cnt + 2` entries. But when we loop through the array to clear it at the - end of this function we only loop until `lno < cnt`, and thus we may not - end up releasing whatever the two extra `sline`s contain. + `cnt + 2` entries: one extra entry for the deletion hunk at the end, and + another entry that we don't seem to ever populate at all but acts as a + kind of sentinel value. - Plug this memory leak. + When we loop through the array to clear it at the end of this function + we only loop until `lno < cnt`, and thus we may not end up releasing + whatever the two extra `sline`s contain. While that shouldn't matter for + the sentinel value, it does matter for the extra deletion hunk sline. + Regardless of that, plug this memory leak by releasing both extra + entries, which makes the logic a bit easier to reason about. + + While at it, fix the formatting of a local comment, which incidentally + also provides the necessary context for why we overallocate the `sline` + array. Signed-off-by: Patrick Steinhardt <ps@pks.im> ## combine-diff.c ## +@@ combine-diff.c: static void show_patch_diff(struct combine_diff_path *elem, int num_parent, + result_file.ptr = result; + result_file.size = result_size; + +- /* Even p_lno[cnt+1] is valid -- that is for the end line number ++ /* ++ * Even p_lno[cnt+1] is valid -- that is for the end line number + * for deletion hunk at the end. + */ + CALLOC_ARRAY(sline[0].p_lno, st_mult(st_add(cnt, 2), num_parent)); @@ combine-diff.c: static void show_patch_diff(struct combine_diff_path *elem, int num_parent, } free(result); 16: d6b96a4012d = 16: b248f266a91 dir: release untracked cache data 17: 3aa6bac5079 = 17: 76e9a6d5792 sparse-index: correctly free EWAH contents 18: 132fe750906 = 18: 70f16486d77 t/helper: stop re-initialization of `the_repository` 19: b8b702eeb28 = 19: f056d31ca50 t/helper: fix leaking buffer in "dump-untracked-cache" 20: ad309ac1b37 = 20: 714c9286e7a dir: fix leak when parsing "status.showUntrackedFiles" 21: 5e243f9ee53 ! 21: 0ff65c1213b builtin/merge: release outbut buffer after performing merge @@ Metadata Author: Patrick Steinhardt <ps@pks.im> ## Commit message ## - builtin/merge: release outbut buffer after performing merge + builtin/merge: release output buffer after performing merge The `obuf` member of `struct merge_options` is used to buffer output in some cases. In order to not discard its allocated memory we only release 22: b75376e3725 = 22: d9e122bb5db list-objects-filter-options: work around reported leak on error