On Thu, Oct 21, 2021 at 09:16:27AM -0700, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> 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. 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
