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

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!