[PATCH] Some small improvements to gencache and netsamlogon_cache
Jeremy Allison
jra at samba.org
Wed Oct 17 17:21:37 UTC 2018
On Wed, Oct 17, 2018 at 04:53:34PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
>
> Review appreciated!
>
> https://gitlab.com/samba-team/devel/samba/pipelines/33257523
LGTM. RB+ and pushed.
Jeremy.
> 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
> 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