[PATCH] A bugfix (IMHO) for namemap_cache
Jeremy Allison
jra at samba.org
Mon Oct 15 17:54:13 UTC 2018
On Mon, Oct 15, 2018 at 03:54:38PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
>
> Unfortunately this failed twice in
>
> [781(4738)/849 at 2h7m25s] samba4.ldap.password_lockout.python(ad_dc_ntvfs)(ad_dc_ntvfs)
>
> so we can't be sure that this is right yet. But nevertheless I would
> appreciate feedback, because I am missing the overview how this patch
> interacts with the AD DC code in a way to kill it.
>
> Anybody with an idea?
I'm taking a vacation day today so I'll look closer
tomorrw, but does this also fail locally or if you
just test samba4.ldap.password_lockout in isolation ?
Cheers,
Jeremy.
> 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
> From 869d395307fea00a7cf065ccd0092ca192205492 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sat, 13 Oct 2018 11:39:03 +0200
> Subject: [PATCH 1/3] winbindd_cache: Fix timeout calculation for sid<->name
> cache
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/winbindd/winbindd_cache.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/source3/winbindd/winbindd_cache.c b/source3/winbindd/winbindd_cache.c
> index 2f3bac7587b..be3e5ffc2ad 100644
> --- a/source3/winbindd/winbindd_cache.c
> +++ b/source3/winbindd/winbindd_cache.c
> @@ -1749,7 +1749,7 @@ static void wcache_name_to_sid_fn(const struct dom_sid *sid,
>
> *state->sid = *sid;
> *state->type = type;
> - state->found = (state->offline || (timeout < time(NULL)));
> + state->found = (state->offline || (timeout > time(NULL)));
> }
>
> static NTSTATUS wcache_name_to_sid(struct winbindd_domain *domain,
> @@ -1884,7 +1884,7 @@ static void wcache_sid_to_name_fn(const char *domain, const char *name,
> return;
> }
> *state->type = type;
> - state->found = (state->offline || (timeout < time(NULL)));
> + state->found = (state->offline || (timeout > time(NULL)));
> }
>
> static NTSTATUS wcache_sid_to_name(struct winbindd_domain *domain,
> --
> 2.11.0
>
>
> From 9c5ac362fac56e42417303dbe3e890ec561fc5cb Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sat, 13 Oct 2018 12:01:41 +0200
> Subject: [PATCH 2/3] namemap_cache: Absorb the expired calculation into
> namemap_cache.c
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/namemap_cache.c | 31 ++++++++++++++++++++++---------
> source3/lib/namemap_cache.h | 12 ++++++++----
> source3/torture/test_namemap_cache.c | 27 ++++++++++++++++++---------
> source3/winbindd/winbindd_cache.c | 13 ++++++++-----
> 4 files changed, 56 insertions(+), 27 deletions(-)
>
> diff --git a/source3/lib/namemap_cache.c b/source3/lib/namemap_cache.c
> index 38f7a2ddc88..f6e71cd6d16 100644
> --- a/source3/lib/namemap_cache.c
> +++ b/source3/lib/namemap_cache.c
> @@ -85,8 +85,10 @@ fail:
> }
>
> struct namemap_cache_find_sid_state {
> - void (*fn)(const char *domain, const char *name,
> - enum lsa_SidType type, time_t timeout,
> + void (*fn)(const char *domain,
> + const char *name,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data);
> void *private_data;
> bool ok;
> @@ -127,15 +129,20 @@ static void namemap_cache_find_sid_parser(time_t timeout, DATA_BLOB blob,
> return;
> }
>
> - state->fn(domain, name, (enum lsa_SidType)type, timeout,
> + state->fn(domain,
> + name,
> + (enum lsa_SidType)type,
> + timeout <= time(NULL),
> state->private_data);
>
> state->ok = true;
> }
>
> bool namemap_cache_find_sid(const struct dom_sid *sid,
> - void (*fn)(const char *domain, const char *name,
> - enum lsa_SidType type, time_t timeout,
> + void (*fn)(const char *domain,
> + const char *name,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data),
> void *private_data)
> {
> @@ -227,7 +234,8 @@ fail:
>
> struct namemap_cache_find_name_state {
> void (*fn)(const struct dom_sid *sid,
> - enum lsa_SidType type, time_t timeout,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data);
> void *private_data;
> bool ok;
> @@ -274,14 +282,19 @@ static void namemap_cache_find_name_parser(time_t timeout, DATA_BLOB blob,
> return;
> }
>
> - state->fn(&sid, (enum lsa_SidType)type, timeout, state->private_data);
> + state->fn(&sid,
> + (enum lsa_SidType)type,
> + timeout <= time(NULL),
> + state->private_data);
>
> state->ok = true;
> }
>
> -bool namemap_cache_find_name(const char *domain, const char *name,
> +bool namemap_cache_find_name(const char *domain,
> + const char *name,
> void (*fn)(const struct dom_sid *sid,
> - enum lsa_SidType type, time_t timeout,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data),
> void *private_data)
> {
> diff --git a/source3/lib/namemap_cache.h b/source3/lib/namemap_cache.h
> index a70de34b885..5de8ce4c0f2 100644
> --- a/source3/lib/namemap_cache.h
> +++ b/source3/lib/namemap_cache.h
> @@ -32,13 +32,17 @@ bool namemap_cache_set_name2sid(const char *domain, const char *name,
> enum lsa_SidType type,
> time_t timeout);
> bool namemap_cache_find_sid(const struct dom_sid *sid,
> - void (*fn)(const char *domain, const char *name,
> - enum lsa_SidType type, time_t timeout,
> + void (*fn)(const char *domain,
> + const char *name,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data),
> void *private_data);
> -bool namemap_cache_find_name(const char *domain, const char *name,
> +bool namemap_cache_find_name(const char *domain,
> + const char *name,
> void (*fn)(const struct dom_sid *sid,
> - enum lsa_SidType type, time_t timeout,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data),
> void *private_data);
>
> diff --git a/source3/torture/test_namemap_cache.c b/source3/torture/test_namemap_cache.c
> index 07c6bf4744e..01dd1b7c8dc 100644
> --- a/source3/torture/test_namemap_cache.c
> +++ b/source3/torture/test_namemap_cache.c
> @@ -26,8 +26,10 @@ static const struct dom_sid domsid = {
> 1, 4, {0,0,0,0,0,5}, {21, 123, 456, 789}
> };
>
> -static void namemap_cache1_fn1(const char *domain, const char *name,
> - enum lsa_SidType type, time_t timeout,
> +static void namemap_cache1_fn1(const char *domain,
> + const char *name,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data)
> {
> bool *p_ok = private_data;
> @@ -41,7 +43,8 @@ static void namemap_cache1_fn1(const char *domain, const char *name,
> }
>
> static void namemap_cache1_fn2(const struct dom_sid *sid,
> - enum lsa_SidType type, time_t timeout,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data)
> {
> bool *p_ok = private_data;
> @@ -53,8 +56,10 @@ static void namemap_cache1_fn2(const struct dom_sid *sid,
> *p_ok = ok;
> }
>
> -static void namemap_cache1_fn3(const char *domain, const char *name,
> - enum lsa_SidType type, time_t timeout,
> +static void namemap_cache1_fn3(const char *domain,
> + const char *name,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data)
> {
> bool *p_ok = private_data;
> @@ -68,7 +73,8 @@ static void namemap_cache1_fn3(const char *domain, const char *name,
> }
>
> static void namemap_cache1_fn4(const struct dom_sid *sid,
> - enum lsa_SidType type, time_t timeout,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data)
> {
> bool *p_ok = private_data;
> @@ -80,8 +86,10 @@ static void namemap_cache1_fn4(const struct dom_sid *sid,
> *p_ok = ok;
> }
>
> -static void namemap_cache1_fn5(const char *domain, const char *name,
> - enum lsa_SidType type, time_t timeout,
> +static void namemap_cache1_fn5(const char *domain,
> + const char *name,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data)
> {
> bool *p_ok = private_data;
> @@ -95,7 +103,8 @@ static void namemap_cache1_fn5(const char *domain, const char *name,
> }
>
> static void namemap_cache1_fn6(const struct dom_sid *sid,
> - enum lsa_SidType type, time_t timeout,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data)
> {
> bool *p_ok = private_data;
> diff --git a/source3/winbindd/winbindd_cache.c b/source3/winbindd/winbindd_cache.c
> index be3e5ffc2ad..7d98d613ac4 100644
> --- a/source3/winbindd/winbindd_cache.c
> +++ b/source3/winbindd/winbindd_cache.c
> @@ -1742,14 +1742,15 @@ struct wcache_name_to_sid_state {
> };
>
> static void wcache_name_to_sid_fn(const struct dom_sid *sid,
> - enum lsa_SidType type, time_t timeout,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data)
> {
> struct wcache_name_to_sid_state *state = private_data;
>
> *state->sid = *sid;
> *state->type = type;
> - state->found = (state->offline || (timeout > time(NULL)));
> + state->found = (!expired || state->offline);
> }
>
> static NTSTATUS wcache_name_to_sid(struct winbindd_domain *domain,
> @@ -1869,8 +1870,10 @@ struct wcache_sid_to_name_state {
> bool found;
> };
>
> -static void wcache_sid_to_name_fn(const char *domain, const char *name,
> - enum lsa_SidType type, time_t timeout,
> +static void wcache_sid_to_name_fn(const char *domain,
> + const char *name,
> + enum lsa_SidType type,
> + bool expired,
> void *private_data)
> {
> struct wcache_sid_to_name_state *state = private_data;
> @@ -1884,7 +1887,7 @@ static void wcache_sid_to_name_fn(const char *domain, const char *name,
> return;
> }
> *state->type = type;
> - state->found = (state->offline || (timeout > time(NULL)));
> + state->found = (!expired || state->offline);
> }
>
> static NTSTATUS wcache_sid_to_name(struct winbindd_domain *domain,
> --
> 2.11.0
>
>
> From 6d551337a957503d5975a7cc08a678c3e8307f17 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sat, 13 Oct 2018 13:41:59 +0200
> Subject: [PATCH 3/3] lib: Move the "expired" for gencache_parse calculation
> into gencache.c
>
> Make it more robust
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/gencache.c | 38 ++++++++++++++++++++++++++------------
> source3/lib/gencache.h | 11 ++++++++++-
> source3/lib/idmap_cache.c | 5 +++--
> source3/lib/namemap_cache.c | 16 ++++++++++------
> source3/lib/util.c | 6 ++++--
> source3/torture/torture.c | 4 +++-
> source3/winbindd/wb_dsgetdcname.c | 6 ++++--
> 7 files changed, 60 insertions(+), 26 deletions(-)
>
> diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
> index b4d374bf837..787ef981ec0 100644
> --- a/source3/lib/gencache.c
> +++ b/source3/lib/gencache.c
> @@ -42,6 +42,14 @@ static struct tdb_wrap *cache_notrans;
> *
> **/
>
> +struct gencache_timeout {
> + time_t timeout;
> +};
> +
> +bool gencache_timeout_expired(const struct gencache_timeout *t)
> +{
> + return t->timeout <= time(NULL);
> +}
>
> /**
> * Cache initialisation function. Opens cache tdb file or creates
> @@ -131,7 +139,8 @@ struct gencache_have_val_state {
> bool gotit;
> };
>
> -static void gencache_have_val_parser(time_t old_timeout, DATA_BLOB data,
> +static void gencache_have_val_parser(const struct gencache_timeout *old_timeout,
> + DATA_BLOB data,
> void *private_data)
> {
> struct gencache_have_val_state *state =
> @@ -146,7 +155,7 @@ static void gencache_have_val_parser(time_t old_timeout, DATA_BLOB data,
> * value, just extending the remaining timeout by less than 10%.
> */
>
> - cache_time_left = old_timeout - now;
> + cache_time_left = old_timeout->timeout - now;
> if (cache_time_left <= 0) {
> /*
> * timed out, write new value
> @@ -338,10 +347,11 @@ done:
> return ret == 0;
> }
>
> -static void gencache_del_parser(time_t timeout, DATA_BLOB blob,
> +static void gencache_del_parser(const struct gencache_timeout *t,
> + DATA_BLOB blob,
> void *private_data)
> {
> - if (timeout != 0) {
> + if (t->timeout != 0) {
> bool *exists = private_data;
> *exists = true;
> }
> @@ -419,7 +429,9 @@ static bool gencache_pull_timeout(uint8_t *val, time_t *pres, char **payload)
> }
>
> struct gencache_parse_state {
> - void (*parser)(time_t timeout, DATA_BLOB blob, void *private_data);
> + void (*parser)(const struct gencache_timeout *timeout,
> + DATA_BLOB blob,
> + void *private_data);
> void *private_data;
> bool copy_to_notrans;
> };
> @@ -428,21 +440,21 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
> {
> struct gencache_parse_state *state;
> DATA_BLOB blob;
> - time_t t;
> + struct gencache_timeout t;
> char *payload;
> bool ret;
>
> if (data.dptr == NULL) {
> return -1;
> }
> - ret = gencache_pull_timeout(data.dptr, &t, &payload);
> + ret = gencache_pull_timeout(data.dptr, &t.timeout, &payload);
> if (!ret) {
> 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, blob, state->private_data);
>
> if (state->copy_to_notrans) {
> tdb_store(cache_notrans->tdb, key, data, 0);
> @@ -452,7 +464,8 @@ static int gencache_parse_fn(TDB_DATA key, TDB_DATA data, void *private_data)
> }
>
> bool gencache_parse(const char *keystr,
> - void (*parser)(time_t timeout, DATA_BLOB blob,
> + void (*parser)(const struct gencache_timeout *timeout,
> + DATA_BLOB blob,
> void *private_data),
> void *private_data)
> {
> @@ -511,17 +524,18 @@ struct gencache_get_data_blob_state {
> bool result;
> };
>
> -static void gencache_get_data_blob_parser(time_t timeout, DATA_BLOB blob,
> +static void gencache_get_data_blob_parser(const struct gencache_timeout *t,
> + DATA_BLOB blob,
> void *private_data)
> {
> struct gencache_get_data_blob_state *state =
> (struct gencache_get_data_blob_state *)private_data;
>
> - if (timeout == 0) {
> + if (t->timeout == 0) {
> state->result = false;
> return;
> }
> - state->timeout = timeout;
> + state->timeout = t->timeout;
>
> if (state->blob == NULL) {
> state->result = true;
> diff --git a/source3/lib/gencache.h b/source3/lib/gencache.h
> index fa72a4aa466..3d8ebe53872 100644
> --- a/source3/lib/gencache.h
> +++ b/source3/lib/gencache.h
> @@ -32,8 +32,17 @@ bool gencache_set(const char *keystr, const char *value, time_t timeout);
> bool gencache_del(const char *keystr);
> bool gencache_get(const char *keystr, TALLOC_CTX *mem_ctx, char **value,
> time_t *ptimeout);
> +
> +/*
> + * This might look like overkill, but namemap_cache.c shows it's
> + * necessary :-)
> + */
> +struct gencache_timeout;
> +bool gencache_timeout_expired(const struct gencache_timeout *t);
> +
> bool gencache_parse(const char *keystr,
> - void (*parser)(time_t timeout, DATA_BLOB blob,
> + void (*parser)(const struct gencache_timeout *timeout,
> + DATA_BLOB blob,
> void *private_data),
> void *private_data);
> bool gencache_get_data_blob(const char *keystr, TALLOC_CTX *mem_ctx,
> diff --git a/source3/lib/idmap_cache.c b/source3/lib/idmap_cache.c
> index 1e8a1ebc607..187723689ad 100644
> --- a/source3/lib/idmap_cache.c
> +++ b/source3/lib/idmap_cache.c
> @@ -194,7 +194,8 @@ struct idmap_cache_xid2sid_state {
> bool ret;
> };
>
> -static void idmap_cache_xid2sid_parser(time_t timeout, DATA_BLOB blob,
> +static void idmap_cache_xid2sid_parser(const struct gencache_timeout *timeout,
> + DATA_BLOB blob,
> void *private_data)
> {
> struct idmap_cache_xid2sid_state *state =
> @@ -217,7 +218,7 @@ static void idmap_cache_xid2sid_parser(time_t timeout, DATA_BLOB blob,
> state->ret = string_to_sid(state->sid, value);
> }
> if (state->ret) {
> - *state->expired = (timeout <= time(NULL));
> + *state->expired = gencache_timeout_expired(timeout);
> }
> }
>
> diff --git a/source3/lib/namemap_cache.c b/source3/lib/namemap_cache.c
> index f6e71cd6d16..c153aa608e2 100644
> --- a/source3/lib/namemap_cache.c
> +++ b/source3/lib/namemap_cache.c
> @@ -94,8 +94,10 @@ struct namemap_cache_find_sid_state {
> bool ok;
> };
>
> -static void namemap_cache_find_sid_parser(time_t timeout, DATA_BLOB blob,
> - void *private_data)
> +static void namemap_cache_find_sid_parser(
> + const struct gencache_timeout *timeout,
> + DATA_BLOB blob,
> + void *private_data)
> {
> struct namemap_cache_find_sid_state *state = private_data;
> const char *strv = (const char *)blob.data;
> @@ -132,7 +134,7 @@ static void namemap_cache_find_sid_parser(time_t timeout, DATA_BLOB blob,
> state->fn(domain,
> name,
> (enum lsa_SidType)type,
> - timeout <= time(NULL),
> + gencache_timeout_expired(timeout),
> state->private_data);
>
> state->ok = true;
> @@ -241,8 +243,10 @@ struct namemap_cache_find_name_state {
> bool ok;
> };
>
> -static void namemap_cache_find_name_parser(time_t timeout, DATA_BLOB blob,
> - void *private_data)
> +static void namemap_cache_find_name_parser(
> + const struct gencache_timeout *timeout,
> + DATA_BLOB blob,
> + void *private_data)
> {
> struct namemap_cache_find_name_state *state = private_data;
> const char *strv = (const char *)blob.data;
> @@ -284,7 +288,7 @@ static void namemap_cache_find_name_parser(time_t timeout, DATA_BLOB blob,
>
> state->fn(&sid,
> (enum lsa_SidType)type,
> - timeout <= time(NULL),
> + gencache_timeout_expired(timeout),
> state->private_data);
>
> state->ok = true;
> diff --git a/source3/lib/util.c b/source3/lib/util.c
> index 394fa5fd191..da8ec55fa61 100644
> --- a/source3/lib/util.c
> +++ b/source3/lib/util.c
> @@ -1242,12 +1242,14 @@ struct ra_parser_state {
> enum remote_arch_types ra;
> };
>
> -static void ra_parser(time_t timeout, DATA_BLOB blob, void *priv_data)
> +static void ra_parser(const struct gencache_timeout *t,
> + DATA_BLOB blob,
> + void *priv_data)
> {
> struct ra_parser_state *state = (struct ra_parser_state *)priv_data;
> const char *ra_str = NULL;
>
> - if (timeout <= time(NULL)) {
> + if (gencache_timeout_expired(t)) {
> return;
> }
>
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index 098560968dd..3053596b439 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -10238,7 +10238,9 @@ static bool run_local_base64(int dummy)
> return ret;
> }
>
> -static void parse_fn(time_t timeout, DATA_BLOB blob, void *private_data)
> +static void parse_fn(const struct gencache_timeout *t,
> + DATA_BLOB blob,
> + void *private_data)
> {
> return;
> }
> diff --git a/source3/winbindd/wb_dsgetdcname.c b/source3/winbindd/wb_dsgetdcname.c
> index 2f450c7a2b4..cbd2cf6fd78 100644
> --- a/source3/winbindd/wb_dsgetdcname.c
> +++ b/source3/winbindd/wb_dsgetdcname.c
> @@ -171,12 +171,14 @@ struct dcinfo_parser_state {
> struct netr_DsRGetDCNameInfo *dcinfo;
> };
>
> -static void dcinfo_parser(time_t timeout, DATA_BLOB blob, void *private_data)
> +static void dcinfo_parser(const struct gencache_timeout *timeout,
> + DATA_BLOB blob,
> + void *private_data)
> {
> struct dcinfo_parser_state *state = private_data;
> enum ndr_err_code ndr_err;
>
> - if (timeout <= time(NULL)) {
> + if (gencache_timeout_expired(timeout)) {
> return;
> }
>
> --
> 2.11.0
>
More information about the samba-technical
mailing list