Taylor Blau [off-list ref] writes:
cleanup_fail:
free(m);
free(midx_name);
- free(cf);
+ free_chunkfile(cf);
if (midx_map)
munmap(midx_map, midx_size);
if (0 <= fd)
Not a fault of this patch, but I think the code is calling close()
on an already closed file descriptor in cleanup_fail codepath, when
"goto cleanup_fail" is reached after xmmap() returned, e.g. when
oid_version() does not match hash_version, when we failed to read
the ToC.
Also, it is not clear why is it a dying offence if we do not find
packnames chunk, but it is just a "pretend as if we do not have this
midx file and everybody else is happy" when we failed to read the
ToC.
On Thu, Oct 21, 2021 at 09:16:27AM -0700, Junio C Hamano wrote:
Taylor Blau [off-list ref] writes:
quoted
cleanup_fail:
free(m);
free(midx_name);
- free(cf);
+ free_chunkfile(cf);
if (midx_map)
munmap(midx_map, midx_size);
if (0 <= fd)
Not a fault of this patch, but I think the code is calling close()
on an already closed file descriptor in cleanup_fail codepath, when
"goto cleanup_fail" is reached after xmmap() returned, e.g. when
oid_version() does not match hash_version, when we failed to read
the ToC.
Also, it is not clear why is it a dying offence if we do not find
packnames chunk, but it is just a "pretend as if we do not have this
midx file and everybody else is happy" when we failed to read the
ToC.
Yep, I agree with you on both of those. I would be happier to see fewer
die()s deep within midx.c. I'll start a list for myself of some
potential future cleanups in this area that don't involve memory leaks.
My hunch is that there's enough subtlty here that we shouldn't tie the
two (fixing leaks, and other general issues/tidiness in the MIDX code)
together. So I'll keep track of the latter separately and address those
in future series.
Thanks,
Taylor