Re: [PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream()

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Subject: object-file: fix mmap() leak in odb_source_loose_read_object_stream()
>
> We mmap() a loose object file, storing the result in the local variable
> "mapped", which is eventually assigned into our stream struct as
> "st.mapped". If we hit an error, we jump to an error label which does:
>
>   munmap(st.mapped, st.mapsize);
>
> to clean up. But this is wrong; we don't assign st.mapped until the end
> of the function, after all of the "goto error" jumps. So this munmap()
> is never cleaning up anything (st.mapped is always NULL, because we
> initialize the struct with calloc).
>
> Instead, we should feed the local variable to munmap().
>
> This leak is due to 595296e124 (streaming: allocate stream inside the
> backend-specific logic, 2025-11-23), which introduced the local
> variable. Before that, we assigned the mmap result directly into
> st.mapped. It was probably switched there so that we do not have to
> allocate/free the struct when the map operation fails (e.g., because we
> don't have the loose object). Before that commit, the struct was passed
> in from the caller, so there was no allocation at all.

Makes sense.  Thanks for finding and fixing the issue so quickly.


>
> You can see the leak in the test suite by building with:
>
>   make SANITIZE=leak NO_MMAP=1 CC=clang
>
> and running t1060. We need NO_MMAP so that the mmap() is backed by an
> actual malloc(), which allows LSan to detect it. And the leak seems not
> to be detected when compiling with gcc, probably due to some internal
> compiler decisions about how the stack memory is written.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  object-file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/object-file.c b/object-file.c
> index 3094140055..ab2fb9c4eb 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2197,7 +2197,7 @@ int odb_source_loose_read_object_stream(struct odb_read_stream **out,
>  	return 0;
>  error:
>  	git_inflate_end(&st->z);
> -	munmap(st->mapped, st->mapsize);
> +	munmap(mapped, mapsize);
>  	free(st);
>  	return -1;
>  }