Patch for a memory leak issue in share mode locking.
Jeremy Allison
jra at samba.org
Wed May 25 23:51:34 UTC 2016
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.
More information about the samba-technical
mailing list