From: Jeff King <peff@peff.net>
Date: Fri, 06 Mar 2026 16:21:06 GMT
Subject: Re: [PATCH 0/4] plugging some mmap() leaks
Message-ID: <20260306162106.GA3483423@coredump.intra.peff.net>
In-Reply-To: <9137fd66-9ac3-42ff-a892-1b6f20b49972@ramsayjones.plus.com>

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. ;)

> 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. :)

This is a clever workaround, but I think we should consider it a bug if
we are calling munmap() twice and fix that.

-Peff