Long-term test runs by QA at Dell suggests that there are memory leaks in the master smbd. Valgrind was used to isolate them. The following seem to be the issues: diff -rupN old/source3/lib/debug.c new/source3/lib/debug.c --- old/source3/lib/debug.c 2012-04-07 06:59:17.000000000 -0700 +++ new/source3/lib/debug.c 2012-05-16 15:01:23.000000000 -0700 @@ -657,9 +657,11 @@ bool reopen_logs( void ) SAFE_FREE(fname); fname = SMB_STRDUP(logfname); if (!fname) { + TALLOC_FREE(logfname); return false; } } + TALLOC_FREE(logfname); } debugf = fname; @@ -1051,18 +1053,17 @@ bool dbghdrclass(int level, int cls, con default_classname_table[cls]); } + char *curtime = current_timestring(talloc_tos(), + lp_debug_hires_timestamp()); /* Print it all out at once to prevent split syslog output. */ if( lp_debug_prefix_timestamp() ) { - (void)Debug1( "[%s, %2d%s] ", - current_timestring(talloc_tos(), - lp_debug_hires_timestamp()), + (void)Debug1( "[%s, %2d%s] ", curtime, level, header_str); } else { - (void)Debug1( "[%s, %2d%s] %s(%s)\n", - current_timestring(talloc_tos(), - lp_debug_hires_timestamp()), + (void)Debug1( "[%s, %2d%s] %s(%s)\n", curtime, level, header_str, location, func ); } + TALLOC_FREE(curtime); } errno = old_errno; diff -rupN old/source3/param/loadparm.c new/source3/param/loadparm.c --- old/source3/param/loadparm.c 2012-04-07 06:59:17.000000000 -0700 +++ new/source3/param/loadparm.c 2012-05-16 11:18:51.000000000 -0700 @@ -9283,7 +9283,9 @@ bool lp_load_ex(const char *pszFname, } } - lp_add_auto_services(lp_auto_services()); + char *serv = lp_auto_services(); + lp_add_auto_services(serv); + TALLOC_FREE(serv); if (add_ipc) { /* When 'restrict anonymous = 2' guest connections to ipc$ diff -rupN old/source3/smbd/server.c new/source3/smbd/server.c --- old/source3/smbd/server.c 2012-04-07 06:59:17.000000000 -0700 +++ new/source3/smbd/server.c 2012-05-16 11:15:00.000000000 -0700 @@ -804,6 +804,7 @@ bool reload_services(bool test) set_dyn_CONFIGFILE(fname); test = False; } + TALLOC_FREE(fname); } reopen_logs(); These appear to occur because lp_string uses talloc_tos() as a context, and it looks like the top stackframe is never cleaned up in the master process.
Great job on this ! Let's make sure they get into the next 3.6.x and 3.5.x. Jeremy.
Adding to 3.6 release bug so we know they'll get in.
Created attachment 7614 [details] git-am fix for 3.5.next Fix for 3.5.next.
Created attachment 7615 [details] git-am fix for 3.6.next Fixes a few more uses than in Richards original patch.
Created attachment 7616 [details] Updated version of Richard's patch for 3.5.next. Also takes care of usage in nmbd and winbindd.
These look good to me ... Also, I should point out, it was my colleague, Shyam Vanga, who prepared the original patches after the problem was found by QA. I just reviewed them to see if they looked reasonable.
Comment on attachment 7616 [details] Updated version of Richard's patch for 3.5.next. Richard ack'ed.
Comment on attachment 7615 [details] git-am fix for 3.6.next Richard acked
There is one more possible issue here: @@ -64,14 +64,26 @@ static const char *charset_name(charset_ ret = "UTF-16BE"; break; case CH_UNIX: - ret = lp_unix_charset(); + { + static char *unix_cs = NULL; + if (!unix_cs) unix_cs = lp_unix_charset(); + ret = unix_cs; break; + } case CH_DOS: - ret = lp_dos_charset(); + { + static char *dos_cs = NULL; + if (!dos_cs) dos_cs = lp_dos_charset(); + ret = dos_cs; break; + } case CH_DISPLAY: - ret = lp_display_charset(); + { + static char *dis_cs = NULL; + if (!dis_cs) dis_cs = lp_display_charset(); + ret = dis_cs; break; + } case CH_UTF8: ret = "UTF8"; break; I am not convinced that this is the best way to fix the issue, however.
I think the best way to fix these is to redefine: lp_display_charset() lp_unix_charset() lp_dos_charset() as FN_GLOBAL_CONST_STRING, which just returns the const pointer without the lp_string call.
(In reply to comment #10) > I think the best way to fix these is to redefine: > > lp_display_charset() > lp_unix_charset() > lp_dos_charset() > > as FN_GLOBAL_CONST_STRING, which just returns the const pointer without the > lp_string call. I agree. That _is_ the best way to fix this problem. Thanks.
Ok, will upload replacement patches with this change, and push to master also. Jeremy.
Created attachment 7617 [details] Latest git-am fix for 3.6.next Including lp_XX_charset() changes.
Created attachment 7618 [details] Latest git-am fix for 3.5.next
OK, I can ack that last change as well. It looks good to me.
Comment on attachment 7617 [details] Latest git-am fix for 3.6.next ACK from Richard.
Comment on attachment 7618 [details] Latest git-am fix for 3.5.next ACK from Richard
Comment on attachment 7618 [details] Latest git-am fix for 3.5.next ACK from Richard.
Re-assigning to Karolin for inclusion in 3.5.next and 3.6.next (acked by Richard, even though he doesn't know how to set the '+' sign in the review dropdown :-). Jeremy.
Pushed to v3-6-test and v3-5-test. Closing out bug report. Thanks!