Memory leak in ntlm_auth ?

Stefan Metzmacher metze at samba.org
Tue Apr 4 09:57:12 UTC 2017


Hi Rebecca,

thanks for the report,
can you please also file a bug at https://bugzilla.samba.org,
this is required to get the fix into the release branches.

While I think your patch fixes the problem, I'd propose the attached
patch for upstream.

Can you test if the attached patch also works for you?

Thanks!
metze

Am 04.04.2017 um 11:20 schrieb Rebecca Gellman via samba-technical:
>  
> 
> Hi, 
> 
> I wish to tell you a tale of investigation and suspicion of a memory
> leak in the ntlm_auth program (as compiled from
> source3/utils/ntlm_auth.c). 
> 
> So in our system, we use ntlm_auth to provide both Kerberos and NTLM
> authentication for transparent (and not-so transparent) web proxy
> connections. In this particular instance, Kerberos is the auth of
> choice, but I believe this is relevant to NTLM too. 
> 
> We spawn up a pool of ntlm_auth processes on demand. If a process is not
> available when a connection comes in, we create a new one up to a
> maximum. 
> 
> The base software version is 4.2.10, as shipped with Debian 8 (Jessie). 
> 
> Now that the particulars of the environment are out of the way..... 
> 
> So we have a customer. And they phoned in a support call that their
> system was falling over. We dive in remotely, and see each ntlm_auth
> process is running at an impressive 1.7GB of memory in the "VIRT" column
> of top. EEP! 
> 
> Suspecting a memory leak, we compiled up version 4.2.10 with a call to
> talloc_enable_leak_reporting_full() in source3/utils/ntlm_auth.c (why
> isn't this a command line option at the least?), along with a mod to our
> own processes to send CTRL+D to ntlm_auth prior to terminating the
> process, and gathering the report on STDERR. 
> 
> Left it running for a day and examined the report. Each process had on
> average over 19,000 DATA_BLOBs of around 1.5K in size, reportedly
> created at lib/util/base64.c line 35, the function
> base64_decode_data_blob_talloc(). 
> 
> After digging through the code, this function is called on (pseudo-code)
> &line_of_STDIN[3] to decode the base64 blob input to a binary blob.
> There's then a load of logic surrounding the 2-letter command code and
> processing gensec, etc, etc, etc. 
> 
> Now... the crux of the matter. If I compare with an earlier (3.x.x)
> branch, we see that data_blob_free() is called on this returned blob
> whenever the command handler returns. In 4.x.x this code has been
> refactored, and the function manage_gensec_request() doesn't always call
> data_blob_free(). In fact, it rarely calls it at all. 
> 
> There's also a couple of returns that omit talloc_free(mem_ctx) that
> seems to precede every other return in this function. 
> 
> So, on this hunch, I added in the missing data_blob_free() calls and the
> two talloc_free(mem_ctx) calls, and rebuilt. Now the same customer
> system is sitting happily at a low memory usage, not showing so much of
> a hint of a memory leak. 
> 
> I've attached the patch we used to "correct" ntlm_auth.c. I'd be
> interested to here some technical commentary from knowledge Samba bods
> :) 
> 
> Regards 
> 
> Rebecca Gellman (On behalf of SmoothWall) 
> 
>   
> 
-------------- next part --------------
From 84ef966e4a33fd770ea35dafb10da5c8bd0a72a9 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 4 Apr 2017 11:52:56 +0200
Subject: [PATCH] s3:ntlm_auth: fix memory leak in manage_gensec_request()

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 source3/utils/ntlm_auth.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/source3/utils/ntlm_auth.c b/source3/utils/ntlm_auth.c
index 6a7a269..5a10e27 100644
--- a/source3/utils/ntlm_auth.c
+++ b/source3/utils/ntlm_auth.c
@@ -1306,6 +1306,8 @@ static void manage_gensec_request(enum stdio_helper_mode stdio_helper_mode,
 
 	TALLOC_CTX *mem_ctx;
 
+	mem_ctx = talloc_named(NULL, 0, "manage_gensec_request internal mem_ctx");
+
 	if (*private1) {
 		state = (struct gensec_ntlm_state *)*private1;
 	} else {
@@ -1323,6 +1325,7 @@ static void manage_gensec_request(enum stdio_helper_mode stdio_helper_mode,
 	if (strlen(buf) < 2) {
 		DEBUG(1, ("query [%s] invalid", buf));
 		printf("BH Query invalid\n");
+		talloc_free(mem_ctx);
 		return;
 	}
 
@@ -1332,9 +1335,10 @@ static void manage_gensec_request(enum stdio_helper_mode stdio_helper_mode,
 			talloc_free(want_feature_list);
 			want_feature_list = talloc_strndup(state, buf+3, strlen(buf)-3);
 			printf("OK\n");
+			talloc_free(mem_ctx);
 			return;
 		}
-		in = base64_decode_data_blob(buf + 3);
+		in = base64_decode_data_blob_talloc(mem_ctx, buf + 3);
 	} else {
 		in = data_blob(NULL, 0);
 	}
@@ -1347,7 +1351,7 @@ static void manage_gensec_request(enum stdio_helper_mode stdio_helper_mode,
 	} else if ( (strncmp(buf, "OK", 2) == 0)) {
 		/* Just return BH, like ntlm_auth from Samba 3 does. */
 		printf("BH Command expected\n");
-		data_blob_free(&in);
+		talloc_free(mem_ctx);
 		return;
 	} else if ( (strncmp(buf, "TT ", 3) != 0) &&
 		    (strncmp(buf, "KK ", 3) != 0) &&
@@ -1359,12 +1363,10 @@ static void manage_gensec_request(enum stdio_helper_mode stdio_helper_mode,
 		    (strncmp(buf, "GF", 2) != 0)) {
 		DEBUG(1, ("SPNEGO request [%s] invalid prefix\n", buf));
 		printf("BH SPNEGO request invalid prefix\n");
-		data_blob_free(&in);
+		talloc_free(mem_ctx);
 		return;
 	}
 
-	mem_ctx = talloc_named(NULL, 0, "manage_gensec_request internal mem_ctx");
-
 	/* setup gensec */
 	if (!(state->gensec_state)) {
 		switch (stdio_helper_mode) {
@@ -1499,7 +1501,6 @@ static void manage_gensec_request(enum stdio_helper_mode stdio_helper_mode,
 					     state->set_password,
 					     CRED_SPECIFIED);
 		printf("OK\n");
-		data_blob_free(&in);
 		talloc_free(mem_ctx);
 		return;
 	}
@@ -1531,10 +1532,12 @@ static void manage_gensec_request(enum stdio_helper_mode stdio_helper_mode,
 		neg_flags = gensec_ntlmssp_neg_flags(state->gensec_state);
 		if (neg_flags == 0) {
 			printf("BH\n");
+			talloc_free(mem_ctx);
 			return;
 		}
 
 		printf("GF 0x%08x\n", neg_flags);
+		talloc_free(mem_ctx);
 		return;
 	}
 
-- 
1.9.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170404/8cf5bd9f/signature.sig>


More information about the samba-technical mailing list