Patch for a memory leak issue in share mode locking.

[Jeremy Allison]

On Wed, May 25, 2016 at 10:27:04AM +0200, Volker Lendecke wrote:
> On Wed, May 25, 2016 at 06:37:59AM +0000, Hemanth Thummala wrote:
> > Hi All,
> > 
> > We have found a memory leak issue in share mode locking.
> > 
> > I have created bug and updated my findings.
> > https://bugzilla.samba.org/show_bug.cgi?id=11934
> > 
> > Attached patch resolves the issue for us. Please let me know if this looks good.
> 
> This is a great finding, thanks!
> 
> However, I don't fully understand why this is not cleaned up in
> unparse_share_modes via the share_mode_memcache_delete() call. If I get
> the talloc hierarchy right then the data blob is a talloc child of "d",
> and sure, the memcache_add_talloc moves "d" to the NULL context.  But the
> hierarchy below should stay intact. So the share_mode_memcache_delete
> should delete any previous blob via the hierarchy.
> 
> Of course we don't need the blob after the store, so your fix is
> right. But the real long-term leak must be somewhere else, your fix will
> only band-aid the deeper issue.

I think I see why.

Because the file is kept open, the struct share_mode_data *d
never gets deleted - it gets bounced between ownership in
the memcache (via:

	TALLOC_FREE()->
		share_mode_data_destructor()->
			share_mode_memcache_store()

and then back to the mem_ctx in:

	get_share_mode_lock()->
		get_share_mode_lock_internal()->
			parse_share_modes()->
				share_mode_memcache_fetch().

inside parse_share_modes():

296         /* See if we already have a cached copy of this key. */
297         d = share_mode_memcache_fetch(mem_ctx, key, &blob);
298         if (d != NULL) {
299                 return d;
300         }

share_mode_memcache_fetch() moves the ownership back onto mem_ctx.

Inside share_mode_memcache_fetch() we have:

        /* Move onto mem_ctx. */
        d = talloc_move(mem_ctx, &ptr);

        /*
         * Now we own d, prevent the cache from freeing it
         * when we delete the entry.
         */
        talloc_set_destructor(d, share_mode_data_nofree_destructor);

        /* Remove from the cache. We own it now. */
        memcache_delete(NULL,
                        SHARE_MODE_LOCK_CACHE,
                        key);

        /* And reset the destructor to none. */
        talloc_set_destructor(d, NULL);

As the file is always open in their test case, when
unparse_share_modes() is called from share_mode_data_destructor()
the struct is owned by the mem_ctx it's been moved onto,
not by the memcache.

The clue to this is in the talloc pool info in the
bug report:

struct memcache                contains 3246739108 bytes in 40519 blocks (ref 0)
        struct memcache_element        contains     95 bytes in   1 blocks (ref 0)
        struct share_mode_data         contains 3246735971 bytes in 40483 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)
            uint8_t                        contains  89088 bytes in   1 blocks (ref 0)

So this is a long winded way of saying that I think that
Hemanth's patch is correct :-).

Reviewed-by: Jeremy Allison <jra at samba.org>.

Volker, do you concur ?

Jeremy.