On Wed, Oct 30, 2013 at 04:43:54PM +0100, Sumit Bose wrote:
> On Wed, Oct 30, 2013 at 04:32:58PM +0100, Jakub Hrozek wrote:
> > On Wed, Oct 30, 2013 at 11:26:21AM -0400, Stephen Gallagher wrote:
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA1
> > >
> > > On 10/30/2013 10:29 AM, Pavel Březina wrote:
> > > >
> > >
> > >
> > > Nack. This happens to work by coincidence (we're initializing the
> > > idmap with a talloc allocator), but the real problem here is that we
> > > don't have a proper free() function to deal with
sss_idmap_unix_to_*()
> > > functions (and friends). We allow the sss_idmap_init() to take a
> > > custom allocator, which means we need a custom free as well. The only
> > > one we offer right now is for freeing the entire idmap_ctx.
> > >
> > > Please add an appropriate sss_idmap_free_data() function and use that.
> >
> > Pavel filed
https://fedorahosted.org/sssd/ticket/2133 to track that
> > effort.
> >
> > I'm not sure it makes too much sense to accept this patch for 1.11.2.
> > Since we're already relying on talloc being the allocator, we can also
> > rely on its hierarchical nature and let it do its job to free the SID..
>
> I'm sorry but this won't work because the TALLOC context used inside the
> idmap code is a long living one because idmapping is needed as long as
> the backend lives. I took this idea for multiple allocators from the
> dhash code but here the hashes might have a shorter lifetime.
>
> bye,
> Sumit
Ah, you're right (and so is Pavel), sorry.
ACK
btw I only acked the patch to make sure the leak is gone for 1.11.2. For
1.11.3 and master we'll introduce a real sss_idmap_free() function.
Pushed to master and sssd-1-11