* [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()
@ 2025-06-04 3:10 Lidong Yan via GitGitGadget
2025-06-04 7:56 ` Patrick Steinhardt
2025-06-05 6:27 ` [PATCH v2] " Lidong Yan via GitGitGadget
0 siblings, 2 replies; 7+ messages in thread
From: Lidong Yan via GitGitGadget @ 2025-06-04 3:10 UTC (permalink / raw)
To: git; +Cc: Lidong Yan, Lidong Yan
From: Lidong Yan <502024330056@smail•nju.edu.cn>
pretty.c:repo_logmsg_reencode() allocated memory should be freed with
repo_unuse_commit_buffer(). Callers sometimes forgot free it at exit
point. Add `repo_unuse_commit_buffer()` in insert_records_from_trailers
at builtin/shortlog.c and create_commit at builtin/replay.c
Signed-off-by: Lidong Yan <502024330056@smail•nju.edu.cn>
---
repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode()
pretty.c:repo_logmsg_reencode() allocated memory should be freed with
repo_unuse_commit_buffer(). Callers sometimes forgot free it at exit
point. Add repo_unuse_commit_buffer() in insert_records_from_trailers at
builtin/shortlog.c and create_commit at builtin/replay.c.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1988%2Fbrandb97%2Ffix-reencode-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1988/brandb97/fix-reencode-leak-v1
Pull-Request: https://github.com/git/git/pull/1988
builtin/replay.c | 1 +
builtin/shortlog.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/replay.c b/builtin/replay.c
index 225cef08807..6172c8aacc9 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo,
obj = parse_object(repo, &ret);
out:
+ repo_unuse_commit_buffer(the_repository, based_on, message);
free_commit_extra_headers(extra);
free_commit_list(parents);
strbuf_release(&msg);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 30075b67be8..dfc7e85ae96 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -186,8 +186,10 @@ static void insert_records_from_trailers(struct shortlog *log,
commit_buffer = repo_logmsg_reencode(the_repository, commit, NULL,
ctx->output_encoding);
body = strstr(commit_buffer, "\n\n");
- if (!body)
+ if (!body) {
+ repo_unuse_commit_buffer(the_repository, commit, commit_buffer);
return;
+ }
trailer_iterator_init(&iter, body);
while (trailer_iterator_advance(&iter)) {
base-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67
--
gitgitgadget
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() 2025-06-04 3:10 [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() Lidong Yan via GitGitGadget @ 2025-06-04 7:56 ` Patrick Steinhardt 2025-06-04 10:50 ` lidongyan 2025-06-05 7:23 ` Jeff King 2025-06-05 6:27 ` [PATCH v2] " Lidong Yan via GitGitGadget 1 sibling, 2 replies; 7+ messages in thread From: Patrick Steinhardt @ 2025-06-04 7:56 UTC (permalink / raw) To: Lidong Yan via GitGitGadget; +Cc: git, Lidong Yan On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote: > diff --git a/builtin/replay.c b/builtin/replay.c > index 225cef08807..6172c8aacc9 100644 > --- a/builtin/replay.c > +++ b/builtin/replay.c > @@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo, > obj = parse_object(repo, &ret); > > out: > + repo_unuse_commit_buffer(the_repository, based_on, message); > free_commit_extra_headers(extra); > free_commit_list(parents); > strbuf_release(&msg); Makes sense. This one _looks_ like a leak that I'd expect to hit in our test suite as it's not part of an error path. > diff --git a/builtin/shortlog.c b/builtin/shortlog.c > index 30075b67be8..dfc7e85ae96 100644 > --- a/builtin/shortlog.c > +++ b/builtin/shortlog.c > @@ -186,8 +186,10 @@ static void insert_records_from_trailers(struct shortlog *log, > commit_buffer = repo_logmsg_reencode(the_repository, commit, NULL, > ctx->output_encoding); > body = strstr(commit_buffer, "\n\n"); > - if (!body) > + if (!body) { > + repo_unuse_commit_buffer(the_repository, commit, commit_buffer); > return; > + } > > trailer_iterator_init(&iter, body); > while (trailer_iterator_advance(&iter)) { Should this one maybe be converted into a `goto out` so that we can release resources in a single location, only? Something like the below patch. Patrick diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 30075b67be8..dd08bc40161 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -177,7 +177,7 @@ static void insert_records_from_trailers(struct shortlog *log, struct strbuf ident = STRBUF_INIT; if (!log->trailers.nr) - return; + goto out; /* * Using repo_format_commit_message("%B") would be simpler here, but @@ -187,7 +187,7 @@ static void insert_records_from_trailers(struct shortlog *log, ctx->output_encoding); body = strstr(commit_buffer, "\n\n"); if (!body) - return; + goto out; trailer_iterator_init(&iter, body); while (trailer_iterator_advance(&iter)) { @@ -206,6 +206,7 @@ static void insert_records_from_trailers(struct shortlog *log, } trailer_iterator_release(&iter); +out: strbuf_release(&ident); repo_unuse_commit_buffer(the_repository, commit, commit_buffer); } ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() 2025-06-04 7:56 ` Patrick Steinhardt @ 2025-06-04 10:50 ` lidongyan 2025-06-05 7:23 ` Jeff King 1 sibling, 0 replies; 7+ messages in thread From: lidongyan @ 2025-06-04 10:50 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: Lidong Yan via GitGitGadget, git 2025年6月4日 15:56，Patrick Steinhardt <ps@pks•im> 写道： > > On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote: >> diff --git a/builtin/replay.c b/builtin/replay.c >> index 225cef08807..6172c8aacc9 100644 >> --- a/builtin/replay.c >> +++ b/builtin/replay.c >> @@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo, >> obj = parse_object(repo, &ret); >> >> out: >> + repo_unuse_commit_buffer(the_repository, based_on, message); >> free_commit_extra_headers(extra); >> free_commit_list(parents); >> strbuf_release(&msg); > > Makes sense. This one _looks_ like a leak that I'd expect to hit in our > test suite as it's not part of an error path. > >> diff --git a/builtin/shortlog.c b/builtin/shortlog.c >> index 30075b67be8..dfc7e85ae96 100644 >> --- a/builtin/shortlog.c >> +++ b/builtin/shortlog.c >> @@ -186,8 +186,10 @@ static void insert_records_from_trailers(struct shortlog *log, >> commit_buffer = repo_logmsg_reencode(the_repository, commit, NULL, >> ctx->output_encoding); >> body = strstr(commit_buffer, "\n\n"); >> - if (!body) >> + if (!body) { >> + repo_unuse_commit_buffer(the_repository, commit, commit_buffer); >> return; >> + } >> >> trailer_iterator_init(&iter, body); >> while (trailer_iterator_advance(&iter)) { > > Should this one maybe be converted into a `goto out` so that we can > release resources in a single location, only? Something like the below > patch. > > Patrick > > diff --git a/builtin/shortlog.c b/builtin/shortlog.c > index 30075b67be8..dd08bc40161 100644 > --- a/builtin/shortlog.c > +++ b/builtin/shortlog.c > @@ -177,7 +177,7 @@ static void insert_records_from_trailers(struct shortlog *log, > struct strbuf ident = STRBUF_INIT; > > if (!log->trailers.nr) > - return; > + goto out; > > /* > * Using repo_format_commit_message("%B") would be simpler here, but > @@ -187,7 +187,7 @@ static void insert_records_from_trailers(struct shortlog *log, > ctx->output_encoding); > body = strstr(commit_buffer, "\n\n"); > if (!body) > - return; > + goto out; > > trailer_iterator_init(&iter, body); > while (trailer_iterator_advance(&iter)) { > @@ -206,6 +206,7 @@ static void insert_records_from_trailers(struct shortlog *log, > } > trailer_iterator_release(&iter); > > +out: > strbuf_release(&ident); > repo_unuse_commit_buffer(the_repository, commit, commit_buffer); > } > Replace return with goto out do looks better. Ident initialized to STRBUF_INIT means ident->alloc = 0, so release on it is also safe. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() 2025-06-04 7:56 ` Patrick Steinhardt 2025-06-04 10:50 ` lidongyan @ 2025-06-05 7:23 ` Jeff King 2025-06-06 12:22 ` lidongyan 1 sibling, 1 reply; 7+ messages in thread From: Jeff King @ 2025-06-05 7:23 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: Lidong Yan via GitGitGadget, git, Lidong Yan On Wed, Jun 04, 2025 at 09:56:39AM +0200, Patrick Steinhardt wrote: > On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote: > > diff --git a/builtin/replay.c b/builtin/replay.c > > index 225cef08807..6172c8aacc9 100644 > > --- a/builtin/replay.c > > +++ b/builtin/replay.c > > @@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo, > > obj = parse_object(repo, &ret); > > > > out: > > + repo_unuse_commit_buffer(the_repository, based_on, message); > > free_commit_extra_headers(extra); > > free_commit_list(parents); > > strbuf_release(&msg); > > Makes sense. This one _looks_ like a leak that I'd expect to hit in our > test suite as it's not part of an error path. We'll usually never flag a leak for commit buffers, because they are stored in (and owned by) a commit-slab. So the memory is not leaked exactly, but we may hold on to it longer than we need to. This mostly only becomes obvious when we do it for every commit in a code path that touches a lot of commits (e.g., "git log" or something). The exception is if we actually had re-encode, which requires a mismatch between the commit and output encodings (which both default to UTF-8). And then it really is a leak. If we add a hack like this: diff --git a/utf8.c b/utf8.c index 35a0251939..d7b7d372c5 100644 --- a/utf8.c +++ b/utf8.c @@ -3,6 +3,7 @@ #include "git-compat-util.h" #include "strbuf.h" #include "utf8.h" +#include "parse.h" /* This code is originally from https://www.cl.cam.ac.uk/~mgk25/ucs/ */ @@ -442,6 +443,12 @@ int is_encoding_utf8(const char *name) int same_encoding(const char *src, const char *dst) { static const char utf8[] = "UTF-8"; + static int always_reencode = -1; + + if (always_reencode < 0) + always_reencode = git_env_bool("GIT_TEST_ALWAYS_REENCODE", 0); + if (always_reencode) + return 0; if (!src) src = utf8; then running: GIT_TEST_ALWAYS_REENCODE=1 make SANITIZE=leak test turns up this leak via t3650-replay-basics.sh (as well as in t6429). It's probably a bit too specialized to carry around as a permanent test mode, though. I thought it might find other cases, but it doesn't. The other one in this patch only triggers when the commit message has no header separator, which is not very likely. > > - if (!body) > > + if (!body) { > > + repo_unuse_commit_buffer(the_repository, commit, commit_buffer); > > return; > > + } > > > > trailer_iterator_init(&iter, body); > > while (trailer_iterator_advance(&iter)) { > > Should this one maybe be converted into a `goto out` so that we can > release resources in a single location, only? Something like the below > patch. Yeah, I think that is nicer, though... > diff --git a/builtin/shortlog.c b/builtin/shortlog.c > index 30075b67be8..dd08bc40161 100644 > --- a/builtin/shortlog.c > +++ b/builtin/shortlog.c > @@ -177,7 +177,7 @@ static void insert_records_from_trailers(struct shortlog *log, > struct strbuf ident = STRBUF_INIT; > > if (!log->trailers.nr) > - return; > + goto out; If you convert this hunk, then we'd look at the uninitialized commit_buffer variable after we jump to the out label. I think the v2 just posted is OK, though (it touches only the one conditional that needs the goto). -Peff ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() 2025-06-05 7:23 ` Jeff King @ 2025-06-06 12:22 ` lidongyan 0 siblings, 0 replies; 7+ messages in thread From: lidongyan @ 2025-06-06 12:22 UTC (permalink / raw) To: Jeff King; +Cc: Patrick Steinhardt, Lidong Yan via GitGitGadget, git 2025年6月5日 15:23，Jeff King <peff@peff•net> 写道： > > On Wed, Jun 04, 2025 at 09:56:39AM +0200, Patrick Steinhardt wrote: > >> On Wed, Jun 04, 2025 at 03:10:07AM +0000, Lidong Yan via GitGitGadget wrote: >>> diff --git a/builtin/replay.c b/builtin/replay.c >>> index 225cef08807..6172c8aacc9 100644 >>> --- a/builtin/replay.c >>> +++ b/builtin/replay.c >>> @@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo, >>> obj = parse_object(repo, &ret); >>> >>> out: >>> + repo_unuse_commit_buffer(the_repository, based_on, message); >>> free_commit_extra_headers(extra); >>> free_commit_list(parents); >>> strbuf_release(&msg); >> >> Makes sense. This one _looks_ like a leak that I'd expect to hit in our >> test suite as it's not part of an error path. > > We'll usually never flag a leak for commit buffers, because they are > stored in (and owned by) a commit-slab. So the memory is not leaked > exactly, but we may hold on to it longer than we need to. This mostly > only becomes obvious when we do it for every commit in a code path that > touches a lot of commits (e.g., "git log" or something). I understand. The static analysis tool which I used to test git find repo_logmsg_reencode() might allocates memory through xstrdup() or reencode_string(), then it report a leak. And I find that xstrdup() is actually dead code. So only reencode_string() may cause leaks. > The exception is if we actually had re-encode, which requires a mismatch > between the commit and output encodings (which both default to UTF-8). > And then it really is a leak. > Agreed. > If we add a hack like this: > > diff --git a/utf8.c b/utf8.c > index 35a0251939..d7b7d372c5 100644 > --- a/utf8.c > +++ b/utf8.c > @@ -3,6 +3,7 @@ > #include "git-compat-util.h" > #include "strbuf.h" > #include "utf8.h" > +#include "parse.h" > > /* This code is originally from https://www.cl.cam.ac.uk/~mgk25/ucs/ */ > > @@ -442,6 +443,12 @@ int is_encoding_utf8(const char *name) > int same_encoding(const char *src, const char *dst) > { > static const char utf8[] = "UTF-8"; > + static int always_reencode = -1; > + > + if (always_reencode < 0) > + always_reencode = git_env_bool("GIT_TEST_ALWAYS_REENCODE", 0); > + if (always_reencode) > + return 0; > > if (!src) > src = utf8; > > then running: > > GIT_TEST_ALWAYS_REENCODE=1 make SANITIZE=leak test > > turns up this leak via t3650-replay-basics.sh (as well as in t6429). > > It's probably a bit too specialized to carry around as a permanent test > mode, though. I thought it might find other cases, but it doesn't. The > other one in this patch only triggers when the commit message has no > header separator, which is not very likely. > >>> - if (!body) >>> + if (!body) { >>> + repo_unuse_commit_buffer(the_repository, commit, commit_buffer); >>> return; >>> + } >>> >>> trailer_iterator_init(&iter, body); >>> while (trailer_iterator_advance(&iter)) { >> >> Should this one maybe be converted into a `goto out` so that we can >> release resources in a single location, only? Something like the below >> patch. > > Yeah, I think that is nicer, though... > >> diff --git a/builtin/shortlog.c b/builtin/shortlog.c >> index 30075b67be8..dd08bc40161 100644 >> --- a/builtin/shortlog.c >> +++ b/builtin/shortlog.c >> @@ -177,7 +177,7 @@ static void insert_records_from_trailers(struct shortlog *log, >> struct strbuf ident = STRBUF_INIT; >> >> if (!log->trailers.nr) >> - return; >> + goto out; > > If you convert this hunk, then we'd look at the uninitialized > commit_buffer variable after we jump to the out label. I think the v2 > just posted is OK, though (it touches only the one conditional that > needs the goto). > > -Peff I actually learn from your hack that GIT_TEST_ is something like GIT_TRACE, both aids to test and debug. Thanks, Lidong ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() 2025-06-04 3:10 [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() Lidong Yan via GitGitGadget 2025-06-04 7:56 ` Patrick Steinhardt @ 2025-06-05 6:27 ` Lidong Yan via GitGitGadget 2025-06-05 7:53 ` Jeff King 1 sibling, 1 reply; 7+ messages in thread From: Lidong Yan via GitGitGadget @ 2025-06-05 6:27 UTC (permalink / raw) To: git; +Cc: Patrick Steinhardt, Lidong Yan, Lidong Yan From: Lidong Yan <502024330056@smail•nju.edu.cn> pretty.c:repo_logmsg_reencode() allocated memory should be freed with repo_unuse_commit_buffer(). Callers sometimes forgot free it at exit point. Add `repo_unuse_commit_buffer()` in insert_records_from_trailers at builtin/shortlog.c and create_commit at builtin/replay.c Signed-off-by: Lidong Yan <502024330056@smail•nju.edu.cn> --- repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() pretty.c:repo_logmsg_reencode() allocated memory should be freed with repo_unuse_commit_buffer(). Callers sometimes forgot free it at exit point. Add repo_unuse_commit_buffer() in insert_records_from_trailers at builtin/shortlog.c and create_commit at builtin/replay.c. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1988%2Fbrandb97%2Ffix-reencode-leak-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1988/brandb97/fix-reencode-leak-v2 Pull-Request: https://github.com/git/git/pull/1988 Range-diff vs v1: 1: a414074f167 ! 1: f5165d6a102 repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() @@ builtin/replay.c: static struct commit *create_commit(struct repository *repo, ## builtin/shortlog.c ## @@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *log, - commit_buffer = repo_logmsg_reencode(the_repository, commit, NULL, ctx->output_encoding); body = strstr(commit_buffer, "\n\n"); -- if (!body) -+ if (!body) { -+ repo_unuse_commit_buffer(the_repository, commit, commit_buffer); - return; -+ } + if (!body) +- return; ++ goto out; trailer_iterator_init(&iter, body); while (trailer_iterator_advance(&iter)) { +@@ builtin/shortlog.c: static void insert_records_from_trailers(struct shortlog *log, + } + trailer_iterator_release(&iter); + ++out: + strbuf_release(&ident); + repo_unuse_commit_buffer(the_repository, commit, commit_buffer); + } builtin/replay.c | 1 + builtin/shortlog.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/replay.c b/builtin/replay.c index 225cef08807..6172c8aacc9 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -84,6 +84,7 @@ static struct commit *create_commit(struct repository *repo, obj = parse_object(repo, &ret); out: + repo_unuse_commit_buffer(the_repository, based_on, message); free_commit_extra_headers(extra); free_commit_list(parents); strbuf_release(&msg); diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 30075b67be8..fe15e114973 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -187,7 +187,7 @@ static void insert_records_from_trailers(struct shortlog *log, ctx->output_encoding); body = strstr(commit_buffer, "\n\n"); if (!body) - return; + goto out; trailer_iterator_init(&iter, body); while (trailer_iterator_advance(&iter)) { @@ -206,6 +206,7 @@ static void insert_records_from_trailers(struct shortlog *log, } trailer_iterator_release(&iter); +out: strbuf_release(&ident); repo_unuse_commit_buffer(the_repository, commit, commit_buffer); } base-commit: 7014b55638da979331baf8dc31c4e1d697cf2d67 -- gitgitgadget ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() 2025-06-05 6:27 ` [PATCH v2] " Lidong Yan via GitGitGadget @ 2025-06-05 7:53 ` Jeff King 0 siblings, 0 replies; 7+ messages in thread From: Jeff King @ 2025-06-05 7:53 UTC (permalink / raw) To: Lidong Yan via GitGitGadget; +Cc: git, Patrick Steinhardt, Lidong Yan On Thu, Jun 05, 2025 at 06:27:26AM +0000, Lidong Yan via GitGitGadget wrote: > From: Lidong Yan <502024330056@smail•nju.edu.cn> > > pretty.c:repo_logmsg_reencode() allocated memory should be freed with > repo_unuse_commit_buffer(). Callers sometimes forgot free it at exit > point. Add `repo_unuse_commit_buffer()` in insert_records_from_trailers > at builtin/shortlog.c and create_commit at builtin/replay.c The patch here looks fine, and I can confirm with the hacky test-patch I showed elsewhere in the thread that the case in replay.c is fixed. I don't think the shortlog one is triggered by the test suite, and it's probably not worth adding a specific test for a commit with no header separator. -Peff ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-06-06 12:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-04 3:10 [PATCH] repo_logmsg_reencode: fix memory leak when use repo_logmsg_reencode() Lidong Yan via GitGitGadget 2025-06-04 7:56 ` Patrick Steinhardt 2025-06-04 10:50 ` lidongyan 2025-06-05 7:23 ` Jeff King 2025-06-06 12:22 ` lidongyan 2025-06-05 6:27 ` [PATCH v2] " Lidong Yan via GitGitGadget 2025-06-05 7:53 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox