From: Ramsay Jones Date: Fri, 06 Mar 2026 17:49:04 GMT Subject: Re: [PATCH 0/4] plugging some mmap() leaks Message-ID: In-Reply-To: <20260306162106.GA3483423@coredump.intra.peff.net> On 06/03/2026 4:21 pm, Jeff King wrote: > 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 ...). ;) >> 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