From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 10 Mar 2026 12:23:53 GMT
Subject: Re: [PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream()
Message-ID: <abANWS_j2g-ae89b@pks.im>
In-Reply-To: <xmqqv7f8td6b.fsf@gitster.g>

On Fri, Mar 06, 2026 at 09:35:08PM -0800, Junio C Hamano wrote:
> 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.

Yup, indeed, this is an obvious fix. Thanks for cleaning up after me!

Patrick