| Message ID | bcd12ecab81029be825a646348cb7ae69970a819.1635282024.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | midx: clean up t5319 under 'SANITIZE=leak' | expand |
Taylor Blau <me@ttaylorr.com> writes: > @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include, > strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot); > > ret = start_command(&cmd); > - if (ret) > + if (ret) { > + child_process_clear(&cmd); > return ret; > + } This happens only when start_command() returns an error. But the function always calls child_process_clear() before doing so. So I am not sure if this hunk is needed. It didn't exist in v1, if I recall correctly. Am I missing something obvious? > @@ -608,9 +612,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix) > struct pack_geometry *geometry = NULL; > struct strbuf line = STRBUF_INIT; > struct tempfile *refs_snapshot = NULL; > - int i, ext, ret; > + int i, ext, ret = 0; > FILE *out; > int show_progress = isatty(2); > + int cmd_cleared = 0; > > /* variables to be filled by option parsing */ > int pack_everything = 0; > @@ -794,7 +799,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) > > ret = start_command(&cmd); > if (ret) > - return ret; > + goto cleanup; Likewise, at this point, start_command() should have already cleared cmd before it returned an error, no? > @@ -818,8 +823,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix) > } > fclose(out); > ret = finish_command(&cmd); > + cmd_cleared = 1; > if (ret) > - return ret; > + goto cleanup; And cmd is also cleared after we pass this point. So perhaps after the cleanup label, there is no need to call child_process_clear() at all, no?
On Wed, Oct 27, 2021 at 04:44:48PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include, > > strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot); > > > > ret = start_command(&cmd); > > - if (ret) > > + if (ret) { > > + child_process_clear(&cmd); > > return ret; > > + } > > This happens only when start_command() returns an error. But the > function always calls child_process_clear() before doing so. > > So I am not sure if this hunk is needed. It didn't exist in v1, if > I recall correctly. Am I missing something obvious? No, it was the person replying to you missing something obvious ;). Any hunks like this that call child_process_clear() after start_command() returns a non-zero value are unnecessary. But the one in repack_promisor_objects() is good, and does prevent the leak that had led me in this direction in the first place. Here is a suitable replacement for this patch (I believe that everything else in this version is fine as-is): --- >8 --- Subject: [PATCH] builtin/repack.c: avoid leaking child arguments `git repack` invokes a handful of child processes: one to write the actual pack, and optionally ones to repack promisor objects and update the MIDX. Most of these are freed automatically by calling `start_command()` (which invokes it on error) and `finish_command()` which calls it automatically. But repack_promisor_objects() can initialize a child_process, populate its array of arguments, and then return from the function before even calling start_command(). Make sure that the prepared list of arguments is freed by calling child_process_clear() ourselves to avoid leaking memory along this path. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/repack.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/repack.c b/builtin/repack.c index 0b2d1e5d82..9b74e0d468 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args, for_each_packed_object(write_oid, &cmd, FOR_EACH_OBJECT_PROMISOR_ONLY); - if (cmd.in == -1) + if (cmd.in == -1) { /* No packed objects; cmd was never started */ + child_process_clear(&cmd); return; + } close(cmd.in); -- 2.33.0.96.g73915697e6
diff --git a/builtin/repack.c b/builtin/repack.c index 0b2d1e5d82..b82f6b485c 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args, for_each_packed_object(write_oid, &cmd, FOR_EACH_OBJECT_PROMISOR_ONLY); - if (cmd.in == -1) + if (cmd.in == -1) { /* No packed objects; cmd was never started */ + child_process_clear(&cmd); return; + } close(cmd.in); @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include, strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot); ret = start_command(&cmd); - if (ret) + if (ret) { + child_process_clear(&cmd); return ret; + } in = xfdopen(cmd.in, "w"); for_each_string_list_item(item, include) @@ -608,9 +612,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix) struct pack_geometry *geometry = NULL; struct strbuf line = STRBUF_INIT; struct tempfile *refs_snapshot = NULL; - int i, ext, ret; + int i, ext, ret = 0; FILE *out; int show_progress = isatty(2); + int cmd_cleared = 0; /* variables to be filled by option parsing */ int pack_everything = 0; @@ -794,7 +799,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) ret = start_command(&cmd); if (ret) - return ret; + goto cleanup; if (geometry) { FILE *in = xfdopen(cmd.in, "w"); @@ -818,8 +823,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix) } fclose(out); ret = finish_command(&cmd); + cmd_cleared = 1; if (ret) - return ret; + goto cleanup; if (!names.nr && !po_args.quiet) printf_ln(_("Nothing new to pack.")); @@ -893,7 +899,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) string_list_clear(&include, 0); if (ret) - return ret; + goto cleanup; } reprepare_packed_git(the_repository); @@ -946,12 +952,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix) write_midx_file(get_object_directory(), NULL, NULL, flags); } +cleanup: string_list_clear(&names, 0); string_list_clear(&rollback, 0); string_list_clear(&existing_nonkept_packs, 0); string_list_clear(&existing_kept_packs, 0); clear_pack_geometry(geometry); strbuf_release(&line); + if (!cmd_cleared) + child_process_clear(&cmd); - return 0; + return ret; }
`git repack` invokes a handful of child processes: one to write the actual pack, and optionally ones to repack promisor objects and update the MIDX. In none of these cases do we bother to call child_process_clear(), which frees the memory associated with each child's arguments and environment. Make sure that we call child_process_clear() in any functions which initialize a struct child_process before returning along a path which did not call finish_command(). In cmd_repack(), take a slightly different approach to use a cleanup label to clear the child_process, unless finish_command() was called. This allows us to free other memory allocated during the lifetime of that function. But it avoids calling child_process_clear() twice (the other call coming from inside of finish_command()) to avoid assuming the function's implementation is idempotent. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/repack.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)