tevent_abort_nesting crash in idmap_ad
jra at samba.org
Sat Jun 25 03:28:08 UTC 2016
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
Metze, Ralph, do you agree ?
More information about the samba-technical