Patch for a memory leak issue in share mode locking.

[Hemanth Thummala]

Thanks Volker ane Jeremy for reviewing the patch.


Yes. We have kept one active file handle open in the test and doing continuous opens and closes on same object. 

Copying Saji, who has actually root caused the memory leak issue. 

Thanks,
Hemanth.

On 5/25/16, 4:51 PM, "Jeremy Allison" <jra at samba.org> wrote:

>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.