Re: [PATCH 0/4] plugging some mmap() leaks
- From
Ramsay Jones <ramsay@ramsayjones.plus.com>
- Date
- Mar 6, 2026, 17:49 UTC
- Message-ID
- <aa83861a-cc0f-4fc2-9599-182dac8b4e9a@ramsayjones.plus.com>
- In-Reply-To
- <20260306162106.GA3483423@coredump.intra.peff.net>
On 06/03/2026 4:21 pm, Jeff King wrote:
Show 25 quoted lines
> On Fri, Mar 06, 2026 at 04:37:49AM +0000, Ramsay Jones wrote: > >> Many moons ago, when the cygwin build routinely set NO_MMAP I had an >> valgrind build of git fail with a 'double free' caused by a call to >> git_munmap() for a pointer that had already been git_munmap-ed! >> >> In addition, the failure was not reproducible (or at least I could not >> find such a test). This was at a time when the testsuite took 4+ hours >> to run for a regular build, let alone a valgrind build. So, to try and >> pin down the failure, I created a debug version of the mmap compat >> functions, which I ran with for several weeks, without failing ... :( >> >> It just so happens that about this time I was also testing running the >> cygwin build without NO_MMAP set. This was a success, so I dropped >> the NO_MMAP investigation, never having found the cause of the failure! > > Interesting. I guess a double-free via munmap() is probably a > harmless-ish noop, rather than a heap corruption. I could believe we > have such a bug somewhere, and it may even be racy (e.g., if it requires > reprepare_packed_git(), or maybe even has to do with stat freshness when > diff.c tries to reuse working tree files). > > We've been testing ASan builds with NO_MMAP for a few months now, so > it's possible that might help flush it out. Though if you ran into it in > 2012, it's possible it has since been unknowingly fixed. ;)
Yep, it was before 2012 and I suspect it has been 'fixed' (but could, of course, still be lingering ...). ;)
Show 9 quoted lines
>> Subject: [PATCH] mmap.c: log mmap() blocks to avoid double-delete bug >> [...] >> In order to guard the implementation from such a calling sequence, >> we keep a list of mmap-block descriptors, which we then consult to >> determine the validity of the input pointer to munmap(). This then >> allows 'git_munmap()' to return -1 on error, as required, with >> errno set to EINVAL. > > Gross. :)
Heh, agreed! :) There were several reasons I didn't submit it in all these years.
> This is a clever workaround, but I think we should consider it a bug if > we are calling munmap() twice and fix that.
Agreed. (I just wanted to bring this to your attention).
ATB, Ramsay Jones