Bug 11708 - Memory leak in loadparm module.
Summary: Memory leak in loadparm module.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.3.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-03 18:44 UTC by Hemanth
Modified: 2021-01-15 10:37 UTC (History)
2 users (show)

See Also:


Attachments
Fix for loadparm memory leak issue. (991 bytes, patch)
2016-02-03 18:49 UTC, Hemanth
no flags Details
git-am fix for 4.4.x, 4.3.x, 4.2.x (1.27 KB, patch)
2016-02-08 20:54 UTC, Jeremy Allison
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hemanth 2016-02-03 18:44:31 UTC
Hi folks,

We are using samba 4.3 stack. We ran into some kind of memory leak issues recently. To detect those leaks, we have run samba under valgrind. We were able to detect and fix one of the leaks specific to us(not in samba).

Whereas, Valgirnd also reported couple of areas as possible memory leak. Both leading to talloc_zero in add_a_service().

…
==32729== 88,320 bytes in 115 blocks are possibly lost in loss record 486 of 487
==32729==    at 0x4C27A2E: malloc (vg_replace_malloc.c:270)
==32729==    by 0x853D91E: __talloc_with_prefix (in /usr/lib/libtalloc.so.2.1.1)
==32729==    by 0x853DAA5: __talloc (in /usr/lib/libtalloc.so.2.1.1)
==32729==    by 0x853DE5C: _talloc_named_const (in /usr/lib/libtalloc.so.2.1.1)
==32729==    by 0x8540941: _talloc_zero (in /usr/lib/libtalloc.so.2.1.1)
==32729==    by 0x6FBFEA2: add_a_service (loadparm.c:1327)
==32729==    by 0x6FC06C7: lp_add_ipc (loadparm.c:1472)
==32729==    by 0x6FC6314: lp_load_ex (loadparm.c:3747)
==32729==    by 0x6FC658B: lp_load (loadparm.c:3806)
==32729==    by 0x534AAD2: reload_services (server_reload.c:146)
==32729==    by 0x53F04A6: check_reload (process.c:2448)
==32729==    by 0x53F1025: housekeeping_fn (process.c:2807)
==32729== 
==32729== 98,304 bytes in 128 blocks are possibly lost in loss record 487 of 487
==32729==    at 0x4C27A2E: malloc (vg_replace_malloc.c:270)
==32729==    by 0x853D91E: __talloc_with_prefix (in /usr/lib/libtalloc.so.2.1.1)
==32729==    by 0x853DAA5: __talloc (in /usr/lib/libtalloc.so.2.1.1)
==32729==    by 0x853DE5C: _talloc_named_const (in /usr/lib/libtalloc.so.2.1.1)
==32729==    by 0x8540941: _talloc_zero (in /usr/lib/libtalloc.so.2.1.1)
==32729==    by 0x6FBFEA2: add_a_service (loadparm.c:1327)
==32729==    by 0x6FC346D: lp_do_section (loadparm.c:2617)
==32729==    by 0x507B118: parse_section (tini.c:199)
==32729==    by 0x507B30A: tini_parse (tini.c:284)
==32729==    by 0x50734F0: pm_process (params.c:99)
==32729==    by 0x6FC605E: lp_load_ex (loadparm.c:3685)
==32729==    by 0x6FC658B: lp_load (loadparm.c:3806)
…

Looking at the code, we have found that NULL context has been used for global ServicePtr structure and for individual service records. This ServicePtrs structure has been freed on gfree_loadparm() and free_service_byindex() is supposed free the actual serviceptr records. 

talloc_free_children(ServicePtrs[idx]) is used in free_service_byindex(). According to its documentation, talloc_free_children() will only cleanup its child contexts and not the actual one.

Instead of using NULL we can use "ServicePtrs” as context for allocating individual records. Valgrind is happy after these changes. No leak reports are seen now.
Comment 1 Hemanth 2016-02-03 18:49:28 UTC
Created attachment 11804 [details]
Fix for loadparm memory leak issue.
Comment 2 Jeremy Allison 2016-02-08 20:54:49 UTC
Created attachment 11813 [details]
git-am fix for 4.4.x, 4.3.x, 4.2.x

Cherry-pick from master. Applies back to 4.2.x.
Comment 3 Andreas Schneider 2016-02-09 09:24:32 UTC
Comment on attachment 11813 [details]
git-am fix for 4.4.x, 4.3.x, 4.2.x

LGTM
Comment 4 Andreas Schneider 2016-02-09 09:24:58 UTC
Karolin, please add the patches to the relevant branches. Thanks!
Comment 5 Karolin Seeger 2016-02-15 09:38:26 UTC
Pushed to autobuild-v4-[4|3|2]-test.
Comment 6 Karolin Seeger 2016-02-17 08:26:07 UTC
(In reply to Karolin Seeger from comment #5)
Pushed to all branches.
Closing out bug report.

Thanks!