Bug 8970 - Possible memory leaks in the samba master process
Summary: Possible memory leaks in the samba master process
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 8595
  Show dependency treegraph
 
Reported: 2012-05-31 19:35 UTC by Richard Sharpe
Modified: 2012-06-13 17:34 UTC (History)
0 users

See Also:


Attachments
git-am fix for 3.5.next (2.53 KB, patch)
2012-05-31 22:44 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.next (3.15 KB, patch)
2012-05-31 23:27 UTC, Jeremy Allison
jra: review+
Details
Updated version of Richard's patch for 3.5.next. (3.67 KB, patch)
2012-05-31 23:32 UTC, Jeremy Allison
jra: review+
Details
Latest git-am fix for 3.6.next (4.59 KB, patch)
2012-06-01 17:49 UTC, Jeremy Allison
jra: review+
Details
Latest git-am fix for 3.5.next (5.07 KB, patch)
2012-06-01 18:06 UTC, Jeremy Allison
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2012-05-31 19:35:35 UTC
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.
Comment 1 Jeremy Allison 2012-05-31 19:37:22 UTC
Great job on this ! Let's make sure they get into the next 3.6.x and 3.5.x.
Jeremy.
Comment 2 Jeremy Allison 2012-05-31 19:37:57 UTC
Adding to 3.6 release bug so we know they'll get in.
Comment 3 Jeremy Allison 2012-05-31 22:44:11 UTC
Created attachment 7614 [details]
git-am fix for 3.5.next

Fix for 3.5.next.
Comment 4 Jeremy Allison 2012-05-31 23:27:31 UTC
Created attachment 7615 [details]
git-am fix for 3.6.next

Fixes a few more uses than in Richards original patch.
Comment 5 Jeremy Allison 2012-05-31 23:32:41 UTC
Created attachment 7616 [details]
Updated version of Richard's patch for 3.5.next.

Also takes care of usage in nmbd and winbindd.
Comment 6 Richard Sharpe 2012-06-01 13:39:52 UTC
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 7 Jeremy Allison 2012-06-01 15:28:16 UTC
Comment on attachment 7616 [details]
Updated version of Richard's patch for 3.5.next.

Richard ack'ed.
Comment 8 Jeremy Allison 2012-06-01 15:28:31 UTC
Comment on attachment 7615 [details]
git-am fix for 3.6.next

Richard acked
Comment 9 Richard Sharpe 2012-06-01 16:47:11 UTC
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.
Comment 10 Jeremy Allison 2012-06-01 17:15:00 UTC
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.
Comment 11 Richard Sharpe 2012-06-01 17:39:20 UTC
(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.
Comment 12 Jeremy Allison 2012-06-01 17:43:05 UTC
Ok, will upload replacement patches with this change, and push to master also.

Jeremy.
Comment 13 Jeremy Allison 2012-06-01 17:49:15 UTC
Created attachment 7617 [details]
Latest git-am fix for 3.6.next

Including lp_XX_charset() changes.
Comment 14 Jeremy Allison 2012-06-01 18:06:02 UTC
Created attachment 7618 [details]
Latest git-am fix for 3.5.next
Comment 15 Richard Sharpe 2012-06-01 18:14:57 UTC
OK, I can ack that last change as well. It looks good to me.
Comment 16 Jeremy Allison 2012-06-01 18:24:51 UTC
Comment on attachment 7617 [details]
Latest git-am fix for 3.6.next

ACK from Richard.
Comment 17 Jeremy Allison 2012-06-01 18:25:06 UTC
Comment on attachment 7618 [details]
Latest git-am fix for 3.5.next

ACK from Richard
Comment 18 Jeremy Allison 2012-06-01 18:25:31 UTC
Comment on attachment 7618 [details]
Latest git-am fix for 3.5.next

ACK from Richard.
Comment 19 Jeremy Allison 2012-06-01 20:34:35 UTC
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.
Comment 20 Karolin Seeger 2012-06-13 17:34:56 UTC
Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!