| Message ID | c477b754e7ddde0d6e696cfd4027ad88c18aeff3.1681850424.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c41258359e2f77a4fba000b2bbb975ebaf7d641b |
| Headers | show |
| Series | gc: enable cruft packs by default | expand |
Taylor Blau <me@ttaylorr.com> writes: > The function `stage_tmp_packfiles()` generates a filename to use for > staging the contents of what will become the pack's ".mtimes" file. > > The name is generated in `write_mtimes_file()` and the result is > returned back to `stage_tmp_packfiles()` which uses it to rename the > temporary file into place via `rename_tmp_packfiles()`. > > `write_mtimes_file()` returns a `const char *`, indicating that callers > are not expected to free its result (similar to, e.g., `oid_to_hex()`). > But callers are expected to free its result, so this return type is > incorrect. Indeed the string that holds the name of the file returned by write_mtimes_file() is leaking. Does the same logic apply to the returned filename from write_rev_file() and stored in rev_tmp_name that is not freed in stage_tmp_packfiles() in another topic? > @@ -544,7 +544,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer, > char **idx_tmp_name) > { > const char *rev_tmp_name = NULL; > - const char *mtimes_tmp_name = NULL; > + char *mtimes_tmp_name = NULL; > > if (adjust_shared_perm(pack_tmp_name)) > die_errno("unable to make temporary pack file readable"); > @@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer, > rename_tmp_packfile(name_buffer, rev_tmp_name, "rev"); > if (mtimes_tmp_name) > rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes"); > + > + free(mtimes_tmp_name); > } > > void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)
On Wed, Apr 19, 2023 at 03:00:48PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > The function `stage_tmp_packfiles()` generates a filename to use for > > staging the contents of what will become the pack's ".mtimes" file. > > > > The name is generated in `write_mtimes_file()` and the result is > > returned back to `stage_tmp_packfiles()` which uses it to rename the > > temporary file into place via `rename_tmp_packfiles()`. > > > > `write_mtimes_file()` returns a `const char *`, indicating that callers > > are not expected to free its result (similar to, e.g., `oid_to_hex()`). > > But callers are expected to free its result, so this return type is > > incorrect. > > Indeed the string that holds the name of the file returned by > write_mtimes_file() is leaking. Does the same logic apply to the > returned filename from write_rev_file() and stored in rev_tmp_name > that is not freed in stage_tmp_packfiles() in another topic? They are similar, but unfortunately different. Here, our temporary name is generated by `write_mtimes_file()` which *always* comes up with a new name from scratch. So we know that it should always be free()'d at the end of `stage_tmp_packfiles()`. But in the case of `write_rev_file()`, it only *sometimes* generates a new name from scratch. The first parameter can be NULL, in which case `write_rev_file()` will generate a new name. Or it can be non-NULL, in which case that name will be used instead. So in that topic, it's less clear on what the ultimate right path forward is. But in this topic, changing `mtimes_tmp_name` and the return type of `write_mtimes_file()` to be a non-const `char *` (and free()ing it, of course ;-)) is the right thing to do. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: >> Indeed the string that holds the name of the file returned by >> write_mtimes_file() is leaking. Does the same logic apply to the >> returned filename from write_rev_file() and stored in rev_tmp_name >> that is not freed in stage_tmp_packfiles() in another topic? > > They are similar, but unfortunately different. I hoped "fortunately different and there is no leak", but it seems what you said below is a bit different X-<. > But in the case of `write_rev_file()`, it only *sometimes* generates a > new name from scratch. The first parameter can be NULL, in which case > `write_rev_file()` will generate a new name. Or it can be non-NULL, in > which case that name will be used instead. IOW, it is a mess. But I think the topic to introduce that variable is being rerolled and has no interaction with the codepath we are discussing here, so we are still in good shape wrt this series ;-) Thanks.
diff --git a/pack-write.c b/pack-write.c index f171405495..4da0ccc5f5 100644 --- a/pack-write.c +++ b/pack-write.c @@ -312,13 +312,13 @@ static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash) hashwrite(f, hash, the_hash_algo->rawsz); } -static const char *write_mtimes_file(struct packing_data *to_pack, - struct pack_idx_entry **objects, - uint32_t nr_objects, - const unsigned char *hash) +static char *write_mtimes_file(struct packing_data *to_pack, + struct pack_idx_entry **objects, + uint32_t nr_objects, + const unsigned char *hash) { struct strbuf tmp_file = STRBUF_INIT; - const char *mtimes_name; + char *mtimes_name; struct hashfile *f; int fd; @@ -544,7 +544,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer, char **idx_tmp_name) { const char *rev_tmp_name = NULL; - const char *mtimes_tmp_name = NULL; + char *mtimes_tmp_name = NULL; if (adjust_shared_perm(pack_tmp_name)) die_errno("unable to make temporary pack file readable"); @@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer, rename_tmp_packfile(name_buffer, rev_tmp_name, "rev"); if (mtimes_tmp_name) rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes"); + + free(mtimes_tmp_name); } void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)
The function `stage_tmp_packfiles()` generates a filename to use for staging the contents of what will become the pack's ".mtimes" file. The name is generated in `write_mtimes_file()` and the result is returned back to `stage_tmp_packfiles()` which uses it to rename the temporary file into place via `rename_tmp_packfiles()`. `write_mtimes_file()` returns a `const char *`, indicating that callers are not expected to free its result (similar to, e.g., `oid_to_hex()`). But callers are expected to free its result, so this return type is incorrect. Change the function's signature to return a non-const `char *`, and free it at the end of `stage_tmp_packfiles()`. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pack-write.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)