tevent_abort_nesting crash in idmap_ad
Ralph Boehme
slow at samba.org
Sat Jun 25 15:13:36 UTC 2016
On Fri, Jun 24, 2016 at 08:28:08PM -0700, Jeremy Allison wrote:
> On Fri, Jun 24, 2016 at 11:21:35PM +0200, Volker Lendecke wrote:
> > On Fri, Jun 24, 2016 at 11:45:01AM -0700, Jeremy Allison wrote:
> > > Was easier than I thought. Compiles but
> > > not (yet) tested.
> >
> > It might fix the nested event loop, but it's pointless and inefficient
> > to go through a _send/_recv pair when we have a sync routine deeply
> > inside. It would make sense if we converted tldap to a fully sync API,
> > but then it would make even more sense to just use the standard, sync
> > OpenLDAP API. The bug that the idmap_ad rewrite fixed can also be fixed
> > using the OpenLDAP API, and that's what I'll have to tackle next.
> >
> > So: NACK until we have a truly async gensec. We can leave the tldap code
> > in if you like, but we need to put in a descriptive #error directive so
> > that people can't use it.
>
> I think that's an unreasonable NAK, given that the alternative
> is complete removal and reversion.
>
> You said "but it's pointless and inefficient to go through a _send/_recv
> pair when we have a sync routine deeply inside."
>
> But that's what the code is *already* doing. The idmap_ad is
> calling the sync version of tldap bind, not the async version,
> All current callers are sync. If you want to make the _send/_recv
> versions static so tldap only exposes the sync version of
> the gensec bind, that's also an option to remove the issue
> of having a sync call inside an async pair.
>
> So the fix is (a) simple, (b) fixes the immediate problem
> and prevents an immediate crash in master. That's a good
> enough reason to +1 it IMHO.
>
> Now this doesn't become urgent as it's master-only right now,
> but I'd like to add this fix into master until you have the
> time to fix it using the OpenLDAP API, or someone fixes the
> gensec SPNEGO to be fully async. Currently the code can crash,
> and that's bad.
>
> Both of these are likely to take some time, and when they're
> done I'm happy for you to either remove tldap, or remove this
> fix. But until then I think this fix is the right thing to
> do.
>
> Metze, Ralph, do you agree ?
yes, sounds like a reasonable fix for me.
Something like the attached hack for gensec to use a instantiated
tevent context upon request in gensec_update_ev() prevents the crash
as well. I still don't understand why gensec_update_send() crashes in
this case, but works from smbd_smb2_session_setup_send().
Cheerio!
-slow
-------------- next part --------------
From 3e5ca344a92c33dc977fcfdd2781f4258dab9962 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 25 Jun 2016 11:18:15 +0200
Subject: [PATCH 1/2] TEST: gensec: add genset_set_use_private_ev()
Use a private tevent context in gensec_update_ev() if requested via
genset_set_use_private_ev().
---
auth/gensec/gensec.c | 47 ++++++++++++++++++++---------------------------
auth/gensec/gensec.h | 2 ++
2 files changed, 22 insertions(+), 27 deletions(-)
diff --git a/auth/gensec/gensec.c b/auth/gensec/gensec.c
index 2a8bba8..797a274 100644
--- a/auth/gensec/gensec.c
+++ b/auth/gensec/gensec.c
@@ -282,23 +282,25 @@ _PUBLIC_ NTSTATUS gensec_update_ev(struct gensec_security *gensec_security,
struct tevent_req *subreq = NULL;
bool ok;
- if (ops->update_send == NULL) {
+ if (ev == NULL || gensec_security->settings->force_private_ev) {
+ frame = talloc_stackframe();
+ ev = samba_tevent_context_init(frame);
if (ev == NULL) {
- frame = talloc_stackframe();
-
- ev = samba_tevent_context_init(frame);
- if (ev == NULL) {
- status = NT_STATUS_NO_MEMORY;
- goto fail;
- }
-
- /*
- * TODO: remove this hack once the backends
- * are fixed.
- */
+ status = NT_STATUS_NO_MEMORY;
+ goto fail;
+ }
+
+ /*
+ * TODO: remove this hack once the backends
+ * are fixed.
+ */
+ if (!gensec_security->settings->force_private_ev) {
tevent_loop_allow_nesting(ev);
}
+ }
+
+ if (ops->update_send == NULL) {
status = ops->update(gensec_security, out_mem_ctx,
ev, in, out);
@@ -325,20 +327,6 @@ _PUBLIC_ NTSTATUS gensec_update_ev(struct gensec_security *gensec_security,
frame = talloc_stackframe();
- if (ev == NULL) {
- ev = samba_tevent_context_init(frame);
- if (ev == NULL) {
- status = NT_STATUS_NO_MEMORY;
- goto fail;
- }
-
- /*
- * TODO: remove this hack once the backends
- * are fixed.
- */
- tevent_loop_allow_nesting(ev);
- }
-
subreq = ops->update_send(frame, ev, gensec_security, in);
if (subreq == NULL) {
status = NT_STATUS_NO_MEMORY;
@@ -755,3 +743,8 @@ _PUBLIC_ const char *gensec_get_target_principal(struct gensec_security *gensec_
return NULL;
}
+
+_PUBLIC_ void genset_set_use_private_ev(struct gensec_security *gensec, bool force_priv_ev)
+{
+ gensec->settings->force_private_ev = force_priv_ev;
+}
diff --git a/auth/gensec/gensec.h b/auth/gensec/gensec.h
index e8bd7b1..147a1ae 100644
--- a/auth/gensec/gensec.h
+++ b/auth/gensec/gensec.h
@@ -95,6 +95,7 @@ struct gensec_settings {
const char *server_dns_name;
const char *server_netbios_domain;
const char *server_netbios_name;
+ bool force_private_ev;
};
struct gensec_security_ops;
@@ -256,5 +257,6 @@ NTSTATUS gensec_generate_session_info_pac(TALLOC_CTX *mem_ctx,
NTSTATUS gensec_magic_check_krb5_oid(struct gensec_security *unused,
const DATA_BLOB *blob);
+void genset_set_use_private_ev(struct gensec_security *gensec, bool force_priv_ev);
#endif /* __GENSEC_H__ */
--
2.5.0
From c2438d07c454177c1c1a8f4d74c450b3821bbc4a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 25 Jun 2016 16:33:06 +0200
Subject: [PATCH 2/2] TEST tldap: use genset_set_use_private_ev
---
source3/lib/tldap_gensec_bind.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/source3/lib/tldap_gensec_bind.c b/source3/lib/tldap_gensec_bind.c
index 07f7956..5c4498a 100644
--- a/source3/lib/tldap_gensec_bind.c
+++ b/source3/lib/tldap_gensec_bind.c
@@ -166,6 +166,8 @@ static void tldap_gensec_bind_got_mechs(struct tevent_req *subreq)
return;
}
+ genset_set_use_private_ev(state->gensec, true);
+
status = gensec_set_credentials(state->gensec, state->creds);
if (!NT_STATUS_IS_OK(status)) {
DBG_DEBUG("gensec_set_credentials failed: %s\n",
--
2.5.0
More information about the samba-technical
mailing list