From: Jacob Keller <jacob.e.keller@intel.com>
Date: Fri, 06 Mar 2026 09:17:24 GMT
Subject: Re: [PATCH 4/4] Makefile: turn on NO_MMAP when building with LSan
Message-ID: <796110ee-d795-4445-9d82-7026370a88cf@intel.com>
In-Reply-To: <20260305231305.GD2901305@coredump.intra.peff.net>

On 3/5/2026 3:13 PM, Jeff King wrote:
> The past few commits fixed some cases where we leak memory allocated by
> mmap(). Building with SANITIZE=leak doesn't detect these because it
> covers only heap buffers allocated by malloc().
> 
> But if we build with NO_MMAP, our compat mmap() implementation will
> allocate a heap buffer and pread() into it. And thus Lsan will detect
> these leaks for free.
> 
> Using NO_MMAP is less performant, of course, since we have to use extra
> memory and read in the whole file, rather than faulting in pages from
> disk. But LSan builds are already slow, and this doesn't make them
> measurably worse. Getting extra coverage for our leak-checking is worth
> it.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index f3264d0a37..4cf1afd395 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS
>  endif
>  ifneq ($(filter leak,$(SANITIZERS)),)
>  BASIC_CFLAGS += -O0
> +NO_MMAP = CatchMapLeaks
>  SANITIZE_LEAK = YesCompiledWithIt
>  endif
>  ifneq ($(filter address,$(SANITIZERS)),)

Should this patch also affect the meson.build?

There is the following in meson.build:

if host_machine.system() == 'windows'
  libgit_c_args += '-DUSE_WIN32_MMAP'
else
  checkfuncs += {
    # provided by compat/mingw.c.
    'unsetenv' : ['unsetenv.c'],
    # provided by compat/mingw.c.
    'getpagesize' : [],
  }

  if get_option('b_sanitize').contains('address')
    libgit_c_args += '-DNO_MMAP'
    libgit_sources += 'compat/mmap.c'
  else
    checkfuncs += { 'mmap': ['mmap.c'] }
  endif
endif

This probably needs to also check if it contains leak, no?

Also I think this might be somewhat less flexible than Make since you
can't forcibly enable mmap even with sanitizers enabled. I suppose thats
not a big deal since enabling sanitizers already has a high cost.

Thanks,
Jake