[PATCH] Some small improvements to gencache and netsamlogon_cache

Volker Lendecke Volker.Lendecke at SerNet.DE
Wed Oct 17 14:53:34 UTC 2018


Hi!

Review appreciated!

https://gitlab.com/samba-team/devel/samba/pipelines/33257523

Thanks, Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From 7bdfc31693343efea90aeff56496baa37a5ecb13 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 13 Oct 2018 10:10:52 +0200
Subject: [PATCH 01/12] netsamlogon_cache: Fix talloc_stackframe error return
 leaks

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/libsmb/samlogon_cache.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/source3/libsmb/samlogon_cache.c b/source3/libsmb/samlogon_cache.c
index 74e89d7c38b..eeb98a17bb0 100644
--- a/source3/libsmb/samlogon_cache.c
+++ b/source3/libsmb/samlogon_cache.c
@@ -134,12 +134,14 @@ bool netsamlogon_cache_store(const char *username, struct netr_SamInfo3 *info3)
 	int ret;
 
 	if (!info3) {
+		TALLOC_FREE(tmp_ctx);
 		return false;
 	}
 
 	if (!netsamlogon_cache_init()) {
 		DEBUG(0,("netsamlogon_cache_store: cannot open %s for write!\n",
 			NETSAMLOGON_TDB));
+		TALLOC_FREE(tmp_ctx);
 		return false;
 	}
 
-- 
2.11.0


From 1e8aae38d77b6b80adcf457269641a4ea6e84c74 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 13 Oct 2018 10:55:00 +0200
Subject: [PATCH 02/12] netsamlogon_cache: Use "goto fail", save some lines

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/libsmb/samlogon_cache.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/source3/libsmb/samlogon_cache.c b/source3/libsmb/samlogon_cache.c
index eeb98a17bb0..b65a1245000 100644
--- a/source3/libsmb/samlogon_cache.c
+++ b/source3/libsmb/samlogon_cache.c
@@ -134,15 +134,13 @@ bool netsamlogon_cache_store(const char *username, struct netr_SamInfo3 *info3)
 	int ret;
 
 	if (!info3) {
-		TALLOC_FREE(tmp_ctx);
-		return false;
+		goto fail;
 	}
 
 	if (!netsamlogon_cache_init()) {
 		DEBUG(0,("netsamlogon_cache_store: cannot open %s for write!\n",
 			NETSAMLOGON_TDB));
-		TALLOC_FREE(tmp_ctx);
-		return false;
+		goto fail;
 	}
 
 	/*
@@ -158,8 +156,7 @@ bool netsamlogon_cache_store(const char *username, struct netr_SamInfo3 *info3)
 	if ((ret == -1) && (tdb_error(netsamlogon_tdb) != TDB_ERR_EXISTS)) {
 		DBG_WARNING("Could not store domain marker for %s: %s\n",
 			    keystr, tdb_errorstr(netsamlogon_tdb));
-		TALLOC_FREE(tmp_ctx);
-		return false;
+		goto fail;
 	}
 
 	sid_compose(&user_sid, info3->base.domain_sid, info3->base.rid);
@@ -207,8 +204,7 @@ bool netsamlogon_cache_store(const char *username, struct netr_SamInfo3 *info3)
 				       (ndr_push_flags_fn_t)ndr_push_netsamlogoncache_entry);
 	if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
 		DEBUG(0,("netsamlogon_cache_store: failed to push entry to cache\n"));
-		TALLOC_FREE(tmp_ctx);
-		return false;
+		goto fail;
 	}
 
 	data.dsize = blob.length;
@@ -218,8 +214,8 @@ bool netsamlogon_cache_store(const char *username, struct netr_SamInfo3 *info3)
 		result = true;
 	}
 
+fail:
 	TALLOC_FREE(tmp_ctx);
-
 	return result;
 }
 
-- 
2.11.0


From 044db17b0ff41c55a83d1025210ec732726fd5a0 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 13 Oct 2018 10:57:13 +0200
Subject: [PATCH 03/12] netsamlogon_cache: Add some error checks

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/libsmb/samlogon_cache.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/source3/libsmb/samlogon_cache.c b/source3/libsmb/samlogon_cache.c
index b65a1245000..d6889775d24 100644
--- a/source3/libsmb/samlogon_cache.c
+++ b/source3/libsmb/samlogon_cache.c
@@ -179,6 +179,9 @@ bool netsamlogon_cache_store(const char *username, struct netr_SamInfo3 *info3)
 
 		if (full_name != NULL) {
 			info3->base.full_name.string = talloc_strdup(info3, full_name);
+			if (info3->base.full_name.string == NULL) {
+				goto fail;
+			}
 		}
 	}
 
@@ -187,6 +190,9 @@ bool netsamlogon_cache_store(const char *username, struct netr_SamInfo3 *info3)
 
 	if (!info3->base.account_name.string) {
 		info3->base.account_name.string = talloc_strdup(info3, username);
+		if (info3->base.account_name.string == NULL) {
+			goto fail;
+		}
 	}
 
 	r.timestamp = time(NULL);
-- 
2.11.0


From ff266cfddd19e4f94589a44640d2cf0310a4b3be Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 13 Oct 2018 10:58:32 +0200
Subject: [PATCH 04/12] netsamlogon_cache: Improve a DBG message

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/libsmb/samlogon_cache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/source3/libsmb/samlogon_cache.c b/source3/libsmb/samlogon_cache.c
index d6889775d24..9638df646f0 100644
--- a/source3/libsmb/samlogon_cache.c
+++ b/source3/libsmb/samlogon_cache.c
@@ -209,7 +209,8 @@ bool netsamlogon_cache_store(const char *username, struct netr_SamInfo3 *info3)
 	ndr_err = ndr_push_struct_blob(&blob, tmp_ctx, &r,
 				       (ndr_push_flags_fn_t)ndr_push_netsamlogoncache_entry);
 	if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
-		DEBUG(0,("netsamlogon_cache_store: failed to push entry to cache\n"));
+		DBG_WARNING("failed to push entry to cache: %s\n",
+			    ndr_errstr(ndr_err));
 		goto fail;
 	}
 
-- 
2.11.0


From fe10b8ee4572514b24edacfe0cf135feb498153e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sat, 13 Oct 2018 10:41:22 +0200
Subject: [PATCH 05/12] auth3: Avoid an explicit ZERO_STRUCT

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/auth/server_info.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/source3/auth/server_info.c b/source3/auth/server_info.c
index 339cce6c4ec..6d5e9fb72fe 100644
--- a/source3/auth/server_info.c
+++ b/source3/auth/server_info.c
@@ -503,7 +503,7 @@ NTSTATUS samu_to_SamInfo3(TALLOC_CTX *mem_ctx,
 	struct netr_SamInfo3 *info3;
 	const struct dom_sid *user_sid;
 	const struct dom_sid *group_sid;
-	struct dom_sid domain_sid;
+	struct dom_sid domain_sid = {0};
 	struct dom_sid *group_sids;
 	uint32_t num_group_sids = 0;
 	const char *tmp;
@@ -523,8 +523,6 @@ NTSTATUS samu_to_SamInfo3(TALLOC_CTX *mem_ctx,
 		return NT_STATUS_NO_MEMORY;
 	}
 
-	ZERO_STRUCT(domain_sid);
-
 	status = SamInfo3_handle_sids(pdb_get_username(samu),
 				user_sid,
 				group_sid,
-- 
2.11.0


From 44dfef56ca53cb91ff78cc87cf9f85fd7ab6beb6 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Mon, 8 Oct 2018 09:07:59 +0200
Subject: [PATCH 06/12] gencache: Avoid counting characters manually

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/gencache.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 787ef981ec0..8919a165745 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -127,10 +127,12 @@ static bool gencache_init(void)
 
 static TDB_DATA last_stabilize_key(void)
 {
-	TDB_DATA result;
-	result.dptr = discard_const_p(uint8_t, "@LAST_STABILIZED");
-	result.dsize = 17;
-	return result;
+	const char key[] = "@LAST_STABILIZED";
+
+	return (TDB_DATA) {
+		.dptr = discard_const_p(uint8_t, key),
+		.dsize = sizeof(key),
+	};
 }
 
 struct gencache_have_val_state {
-- 
2.11.0


From a02e9d33d02b9145b37edaaefd9af32fd2c1a0e4 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 9 Oct 2018 13:15:22 +0200
Subject: [PATCH 07/12] gencache: Swap tests: Do cheapest first

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/gencache.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 8919a165745..895c9cc05a7 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -273,13 +273,13 @@ bool gencache_set_data_blob(const char *keystr, DATA_BLOB blob,
 	static int writecount;
 	TDB_DATA dbufs[2];
 
-	if (tdb_data_cmp(string_term_tdb_data(keystr),
-			 last_stabilize_key()) == 0) {
-		DEBUG(10, ("Can't store %s as a key\n", keystr));
+	if ((keystr == NULL) || (blob.data == NULL)) {
 		return false;
 	}
 
-	if ((keystr == NULL) || (blob.data == NULL)) {
+	if (tdb_data_cmp(string_term_tdb_data(keystr),
+			 last_stabilize_key()) == 0) {
+		DEBUG(10, ("Can't store %s as a key\n", keystr));
 		return false;
 	}
 
-- 
2.11.0


From 032caaf94238731c183b1aaa61912f4fcdaa8a19 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 9 Oct 2018 13:17:53 +0200
Subject: [PATCH 08/12] gencache: Call string_term_tdb_data() only once

---
 source3/lib/gencache.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 895c9cc05a7..f18d0426d81 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -266,6 +266,7 @@ static int last_stabilize_parser(TDB_DATA key, TDB_DATA data,
 bool gencache_set_data_blob(const char *keystr, DATA_BLOB blob,
 			    time_t timeout)
 {
+	TDB_DATA key;
 	int ret;
 	fstring hdr;
 	int hdr_len;
@@ -277,8 +278,10 @@ bool gencache_set_data_blob(const char *keystr, DATA_BLOB blob,
 		return false;
 	}
 
-	if (tdb_data_cmp(string_term_tdb_data(keystr),
-			 last_stabilize_key()) == 0) {
+	key = string_term_tdb_data(keystr);
+
+	ret = tdb_data_cmp(key, last_stabilize_key());
+	if (ret == 0) {
 		DEBUG(10, ("Can't store %s as a key\n", keystr));
 		return false;
 	}
@@ -311,8 +314,7 @@ bool gencache_set_data_blob(const char *keystr, DATA_BLOB blob,
 		   (int)(timeout - time(NULL)), 
 		   timeout > time(NULL) ? "ahead" : "in the past"));
 
-	ret = tdb_storev(cache_notrans->tdb, string_term_tdb_data(keystr),
-			 dbufs, 2, 0);
+	ret = tdb_storev(cache_notrans->tdb, key, dbufs, 2, 0);
 	if (ret != 0) {
 		return false;
 	}
-- 
2.11.0


From 418c186ba04b9df2b66b2a7f8f89b91908408a92 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 9 Oct 2018 13:51:46 +0200
Subject: [PATCH 09/12] gencache: Make gencache_pull_timeout a bit more robust

The previous version assumed a well-formed "val", we just handed it to
strtol without properly checking that it contains the delimiter. So
strtol could well run off the end of "val" in case of data corruption.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/gencache.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index f18d0426d81..d72aa9505af 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -408,19 +408,24 @@ bool gencache_del(const char *keystr)
 	return result;
 }
 
-static bool gencache_pull_timeout(uint8_t *val, time_t *pres, char **payload)
+static bool gencache_pull_timeout(TDB_DATA data, time_t *pres, char **payload)
 {
 	time_t res;
+	char *slash = NULL;
 	char *endptr;
 
-	if (val == NULL) {
+	if (data.dptr == NULL) {
+		return false;
+	}
+	slash = memchr(data.dptr, '/', data.dsize);
+	if (slash == NULL) {
 		return false;
 	}
 
-	res = strtol((char *)val, &endptr, 10);
+	res = strtol((char *)data.dptr, &endptr, 10);
 
 	if ((endptr == NULL) || (*endptr != '/')) {
-		DEBUG(2, ("Invalid gencache data format: %s\n", (char *)val));
+		DBG_WARNING("Invalid gencache data format\n");
 		return false;
 	}
 	if (pres != NULL) {
@@ -451,7 +456,7 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
 	if (data.dptr == NULL) {
 		return -1;
 	}
-	ret = gencache_pull_timeout(data.dptr, &t.timeout, &payload);
+	ret = gencache_pull_timeout(data, &t.timeout, &payload);
 	if (!ret) {
 		return -1;
 	}
@@ -716,7 +721,7 @@ static int stabilize_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
 		return 0;
 	}
 
-	if (!gencache_pull_timeout(val.dptr, &timeout, NULL)) {
+	if (!gencache_pull_timeout(val, &timeout, NULL)) {
 		DEBUG(10, ("Ignoring invalid entry\n"));
 		return 0;
 	}
@@ -841,7 +846,7 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
 		}
 	}
 
-	if (!gencache_pull_timeout(data.dptr, &timeout, &payload)) {
+	if (!gencache_pull_timeout(data, &timeout, &payload)) {
 		goto done;
 	}
 
-- 
2.11.0


From a135b6a0091b80063e7292ead0b094265d398b1b Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 9 Oct 2018 13:58:43 +0200
Subject: [PATCH 10/12] gencache: Make gencache_pull_timeout return a payload
 DATA_BLOB

Both relevant callers created one anyway.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/gencache.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index d72aa9505af..eb268ad3ad1 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -408,7 +408,7 @@ bool gencache_del(const char *keystr)
 	return result;
 }
 
-static bool gencache_pull_timeout(TDB_DATA data, time_t *pres, char **payload)
+static bool gencache_pull_timeout(TDB_DATA data, time_t *pres, DATA_BLOB *payload)
 {
 	time_t res;
 	char *slash = NULL;
@@ -432,7 +432,11 @@ static bool gencache_pull_timeout(TDB_DATA data, time_t *pres, char **payload)
 		*pres = res;
 	}
 	if (payload != NULL) {
-		*payload = endptr+1;
+		endptr += 1;
+		*payload = (DATA_BLOB) {
+			.data = discard_const_p(uint8_t, endptr),
+			.length = data.dsize - PTR_DIFF(endptr, data.dptr),
+		};
 	}
 	return true;
 }
@@ -448,9 +452,8 @@ struct gencache_parse_state {
 static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
 {
 	struct gencache_parse_state *state;
-	DATA_BLOB blob;
 	struct gencache_timeout t;
-	char *payload;
+	DATA_BLOB payload;
 	bool ret;
 
 	if (data.dptr == NULL) {
@@ -461,9 +464,7 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
 		return -1;
 	}
 	state = (struct gencache_parse_state *)private_data;
-	blob = data_blob_const(
-		payload, data.dsize - PTR_DIFF(payload, data.dptr));
-	state->parser(&t, blob, state->private_data);
+	state->parser(&t, payload, state->private_data);
 
 	if (state->copy_to_notrans) {
 		tdb_store(cache_notrans->tdb, key, data, 0);
@@ -826,7 +827,7 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
 	char *keystr;
 	char *free_key = NULL;
 	time_t timeout;
-	char *payload;
+	DATA_BLOB payload;
 
 	if (tdb_data_cmp(key, last_stabilize_key()) == 0) {
 		return 0;
@@ -863,10 +864,7 @@ static int gencache_iterate_blobs_fn(struct tdb_context *tdb, TDB_DATA key,
 		   "(key=[%s], timeout=[%s])\n",
 		   keystr, timestring(talloc_tos(), timeout)));
 
-	state->fn(keystr,
-		  data_blob_const(payload,
-				  data.dsize - PTR_DIFF(payload, data.dptr)),
-		  timeout, state->private_data);
+	state->fn(keystr, payload, timeout, state->private_data);
 
  done:
 	TALLOC_FREE(free_key);
-- 
2.11.0


From 343ae3f3e93877c78f3a222768b1383887ec8de0 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 9 Oct 2018 14:04:50 +0200
Subject: [PATCH 11/12] gencache: Remove a redundant check

gencache_pull_timeout checks for NULL ptr already

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/gencache.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index eb268ad3ad1..528414bd66e 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -456,9 +456,6 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
 	DATA_BLOB payload;
 	bool ret;
 
-	if (data.dptr == NULL) {
-		return -1;
-	}
 	ret = gencache_pull_timeout(data, &t.timeout, &payload);
 	if (!ret) {
 		return -1;
-- 
2.11.0


From 8e76c5a04515a34a0a1cbdf14e765fa0bb513722 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Tue, 9 Oct 2018 21:41:52 +0200
Subject: [PATCH 12/12] gencache: Remove a redundant check

tdb_storev itself is robust against overflow due to multiple buffers

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/lib/gencache.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
index 528414bd66e..6f7734c2004 100644
--- a/source3/lib/gencache.c
+++ b/source3/lib/gencache.c
@@ -301,9 +301,6 @@ bool gencache_set_data_blob(const char *keystr, DATA_BLOB blob,
 	if (hdr_len == -1) {
 		return false;
 	}
-	if ((blob.length + (size_t)hdr_len) < blob.length) {
-		return false;
-	}
 
 	dbufs[0] = (TDB_DATA) { .dptr = (uint8_t *)hdr, .dsize = hdr_len };
 	dbufs[1] = (TDB_DATA) { .dptr = blob.data, .dsize = blob.length };
-- 
2.11.0



More information about the samba-technical mailing list