Bug 10759 - Memory leak in libsmbclient in cli_set_mntpoint function
Summary: Memory leak in libsmbclient in cli_set_mntpoint function
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 3.6.3
Hardware: All All
: P5 major
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-06 06:31 UTC by hargagan
Modified: 2014-09-03 07:14 UTC (History)
1 user (show)

See Also:

Attachments
Proposed patch fixing the issue of memory leak in cli_set_mntpoint() in libsmbclient.so (673 bytes, patch)
2014-08-06 06:34 UTC, hargagan 		no flags Details
Patch generated as suggested in comment #2 by Stefan (1.02 KB, patch)
2014-08-06 09:08 UTC, hargagan 		no flags Details
git-am fix for 4.1.next and 4.0.next. (1.41 KB, patch)
2014-08-13 17:40 UTC, Jeremy Allison 		vl: review+
Details
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description hargagan 2014-08-06 06:31:12 UTC 
During our testing with libsmbclient we found some memory leaks with valgrind. The relevant valgrind logs are :

==29641== 5,670 bytes in 54 blocks are definitely lost in loss record 862 of 910
==29641==    at 0x4C27F9E: malloc (vg_replace_malloc.c:291)
==29641==    by 0x11300B89: talloc_strdup (in /usr/lib64/libtalloc.so.2.0.5)
==29641==    by 0x10C05E84: talloc_string_sub2 (in /usr/lib64/libsmbclient.so.0)
==29641==    by 0x10C06175: talloc_all_string_sub (in /usr/lib64/libsmbclient.so.0)
==29641==    by 0x10C0CC89: clean_name (in /usr/lib64/libsmbclient.so.0)
==29641==    by 0x10C5F637: cli_set_mntpoint (in /usr/lib64/libsmbclient.so.0)
==29641==    by 0x10C61604: cli_resolve_path (in /usr/lib64/libsmbclient.so.0)
==29641==    by 0x10BB5DF4: SMBC_getatr (in /usr/lib64/libsmbclient.so.0)
==29641==    by 0x10BBAD16: SMBC_stat_ctx (in /usr/lib64/libsmbclient.so.0)

Attaching the fix which is working for me. Please review it.
Comment 1 hargagan 2014-08-06 06:34:05 UTC 
Created attachment 10177 [details]
Proposed patch fixing the issue of memory leak in cli_set_mntpoint() in libsmbclient.so

The fix is done to use a stack frame and that is passed to clean_name(). Otherwise the passed 'NULL' context was not freeing the intermediate memory allocated in the code flow in clean_name() and later.
Comment 2 Stefan Metzmacher 2014-08-06 07:00:43 UTC 
(In reply to comment #1)
> Created attachment 10177 [details]
> Proposed patch fixing the issue of memory leak in cli_set_mntpoint() in
> libsmbclient.so
> 
> The fix is done to use a stack frame and that is passed to clean_name().
> Otherwise the passed 'NULL' context was not freeing the intermediate memory
> allocated in the code flow in clean_name() and later.

The change look good to me.

Can you create a real git commit with a Signed-off-by:
and attach the file created by 'git format-patch -1'.
Comment 3 Volker Lendecke 2014-08-06 07:05:02 UTC 
The patch will certainly fix it, but I would also like the callees to not leak temporary memory.
Comment 4 hargagan 2014-08-06 09:08:11 UTC 
Created attachment 10178 [details]
Patch generated as suggested in comment #2 by Stefan
Comment 5 Stefan Metzmacher 2014-08-07 07:09:16 UTC 
(In reply to comment #3)
> The patch will certainly fix it, but I would also like the callees to not leak
> temporary memory.

Yes, all *clean_name() functions seem to have a similar problem
and it might be better to fix the problem there...
Comment 6 Volker Lendecke 2014-08-07 07:22:55 UTC 
Yesterday I've already taken a brief look, but if you want to fix this first, feel free.
Comment 7 Stefan Metzmacher 2014-08-07 07:56:16 UTC 
(In reply to comment #6)
> Yesterday I've already taken a brief look, but if you want to fix this first,
> feel free.

It's a bit of work and I don't have to time for this at the moment...
Comment 8 Jeremy Allison 2014-08-13 00:07:51 UTC 
(In reply to comment #4)
> Created attachment 10178 [details]
> Patch generated as suggested in comment #2 by Stefan

Pushed to master with Metze's, Volker and my 'Reviewed-by:'.

Volker has a test patch to tidy up clean_name() but we don't need that to fix this bug.

I'll upload a back-port to 4.0.next, 4.1.next once the autobuild goes through.

Jeremy.
Comment 9 Jeremy Allison 2014-08-13 17:40:54 UTC 
Created attachment 10199 [details]
git-am fix for 4.1.next and 4.0.next.

Patch that went into master. Applies cleanly to 4.1.next, 4.0.next.
Comment 10 Jeremy Allison 2014-08-13 18:29:28 UTC 
Re-assigning to Karolin for inclusion in 4.1.next, 4.0.next.
Comment 11 Karolin Seeger 2014-09-01 18:52:54 UTC 
Pushed to autobuild-v4-[0|1]-test.
Comment 12 Karolin Seeger 2014-09-03 07:14:21 UTC 
Pushed to both branches.
Closing out bug report.

Thanks!