Subject: Re: [PATCH 0/4] plugging some mmap() leaks

From: Ramsay Jones <[email-masked]>
Date: Fri, 6 Mar 2026 17:49:04 +0000

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